Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

database/sql: add usage examples for (*Rows).NextResultSet #29973

Closed
SagarPuneria opened this issue Jan 29, 2019 · 13 comments
Closed

database/sql: add usage examples for (*Rows).NextResultSet #29973

SagarPuneria opened this issue Jan 29, 2019 · 13 comments
Labels
Documentation FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@SagarPuneria
Copy link

What version of Go are you using (go version)?

go version go1.11.5 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Windows 10, 64 bit

What did you do?

I am using func (rs *Rows) NextResultSet() bool in my code to check whether there is further result sets in table and in my table there is more one than records. So immediately after reading one record i am returning from for rows.Next() {} to test NextResultSet() bool
db Query example

What did you expect to see?

Output:
DNS: root:root@tcp(127.0.0.1:3306)/testdb
Connection Established
PersonID: 32 LastName: Tom FirstName: Erichsen Address: Stavanger City: Norway
2019/01/29 12:57:31 expected more result sets

What did you see instead?

Output:
DNS: root:root@tcp(127.0.0.1:3306)/testdb
Connection Established
PersonID: 32 LastName: Tom FirstName: Erichsen Address: Stavanger City: Norway

@SagarPuneria
Copy link
Author

CC @gopherbot @ALTree

@SagarPuneria
Copy link
Author

SagarPuneria commented Jan 29, 2019

After debugging method NextResultSet() in database/sql:

// NextResultSet prepares the next result set for reading. It returns true if
// there is further result sets, or false if there is no further result set
// or if there is an error advancing to it. The Err method should be consulted
// to distinguish between the two cases.
//
// After calling NextResultSet, the Next method should always be called before
// scanning. If there are further result sets they may not have rows in the result
// set.
func (rs *Rows) NextResultSet() bool {
	var doClose bool
	defer func() {
		if doClose {
			rs.Close()
		}
	}()
	rs.closemu.RLock()
	defer rs.closemu.RUnlock()

	if rs.closed {
		return false
	}

	rs.lastcols = nil
	nextResultSet, ok := rs.rowsi.(driver.RowsNextResultSet)
	if !ok {
		doClose = true
		return false
	}

	// Lock the driver connection before calling the driver interface
	// rowsi to prevent a Tx from rolling back the connection at the same time.
	rs.dc.Lock()
	defer rs.dc.Unlock()

	rs.lasterr = nextResultSet.NextResultSet()
	if rs.lasterr != nil {
		doClose = true
		return false
	}
	return true
}

From above code error returning from function nextResultSet.NextResultSet() is not properly handled. For more information read documentation for type RowsNextResultSet interface and it is clearly mentioned that NextResultSet should return io.EOF when there are no more result sets. I think it would be better to refactor error handling code something like this:

if err := rs.lasterr; err != nil && err != io.EOF {
	doClose = true
	return false
}

@renthraysk
Copy link

The code provided has an unconditional break within the for rows.Next() {} loop. So will only print one row.

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2019

CC @bradfitz @kardianos @kevinburke

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2019

Result sets are sets of records. Your example appears to only issue one simple query (SELECT * FROM persons): why do you expect it to return multiple result sets?

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 29, 2019
@SagarPuneria
Copy link
Author

SagarPuneria commented Jan 30, 2019

Actually the break statement must be inside error handling in my example some thing like:

if err := rows.Scan(&PersonID, &LastName, &FirstName, &Address, &City); err != nil {
        fmt.Println("Error in Scan, Error Info:", err)
        break
}
fmt.Println("PersonID:", PersonID, "LastName:", LastName, "FirstName:", FirstName, "Address:", Address, "City:", City)

In my example for simplicity i used one simple query. You can use any SELECT query try that case which i shown in example after reading first row in table suppose if there is any error in rows.Scan then i don't want to scan next row so i break from for loop. And i expect to use NextResultSet to check whether there is any further result sets in table or not from where i break it.

@kardianos
Copy link
Contributor

Im not following your issue at all.
Please use https://bitbucket.org/kardianos/table

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2019

@SagarPuneria, from what I can tell, NextResultSet is behaving exactly as desisgned and documented: you're issuing one query to the database, and it is returning one result set corresponding to that query. I suspect that the problem here is confusion over the API rather than an actual bug in the database/sql package.

I was going to point you to some examples of queries where NextResultSet is useful, but I notice that we don't seem to have any such examples in the package docs or the corresponding wiki page (https://golang.org/s/sqlwiki). So I'll retitle the bug to describe the documentation issue, and in the meantime you might want to have a look at the motivating examples in #12382.

@bcmills bcmills changed the title database/sql: NextResultSet always return false even if there is further result sets in table database/sql: add usage examples for (*Rows).NextResultSet Jan 30, 2019
@bcmills bcmills added Documentation help wanted and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jan 30, 2019
@bcmills bcmills added this to the Unplanned milestone Jan 30, 2019
@SagarPuneria
Copy link
Author

SagarPuneria commented Jan 31, 2019

@bcmills, let me clear, i am new to database, it seems you got confused. NextResultSet example was already given in db.Query example.

Lets come to the point, the reason behind escalating this issue is that i want to know in which case NextResultSet return true?

I tried all the cases using mysql database it is always returning false. If so then what is use case of implementing NextResultSet?
Could you please give me at least one case where NextResultSet return true.

Before giving your answer, i would suggest you to do sample program on NextResultSet using mysql database where it can return true case.

I hope you got my Question.

@renthraysk
Copy link

Calls a stored procedure that returns multiple resultsets on mysql using github.com/go-sql-driver/mysql

https://gist.github.com/renthraysk/861bfb6f64e1b5d8e0d8b4d6e8b95be0

@SagarPuneria
Copy link
Author

SagarPuneria commented Feb 1, 2019

@renthraysk, yes you are right. Once it returns multiple resultsets it means it reaches the EOF when there are no more result sets then NextResultSet return false as per golang internal code.

But my question in which case the API NextResultSet can return true case using mysql database?

Suppose If NextResultSet API never return true then what is the use case of implementing this API ?

@SagarPuneria
Copy link
Author

SagarPuneria commented Feb 1, 2019

CC @bcmills, @renthraysk
Just for more information, I want to do comparision between NextResultSet and Next API based on the documentation provided.

func (*Rows) Next() bool:
Next method returns true on success, or false if there is no next result row.

func (*Rows) NextResultSet() bool:
NextResultSet returns true if there is further result sets, or false if there is no further result set.

Now in MySQL database:

  1. Next method returning true if next row exist: Next method is behaving exactly as designed and documented
  2. NextResultSet method returning false even if there is further result sets: NextResultSet method is NOT behaving exactly as designed and documented

Why NextResultSet method is NOT behaving exactly as designed and documentation?

As per documentation provided for type RowsNextResultSet interface, it is mentioned that NextResultSet should return io.EOF when there are no more result sets.
Might be developer assumed that if return error is io.EOF then there are no more result sets. But its not true in every case, even if there are further result sets it return io.EOF error, because after reading the entire/multiple result set only function NextResultSet defined for type RowsNextResultSet interface return io.EOF(End Of File) error.
Due to this return error io.EOF in rs.lasterr = nextResultSet.NextResultSet() is not behaving exactly as documented.

Solution:

I think it would be better to refactor error handling code

Finally I would suggest that change either the documentation or code.

If you want to change the code defined in NextResultSet method: i would suggest to refactor error handling code something like:

rs.lasterr = nextResultSet.NextResultSet()
if err := rs.lasterr; err != nil && err != io.EOF {
	doClose = true
	return false
}

@SagarPuneria
Copy link
Author

Its was my big mistake. There is NO issue in NextResultSet method.

I was working with different databases using golang sql package.
In PostgreSQL this NextResultSet method returns true using (github.com/lib/pq) even there is no multiple resultsets.
Sample program

I was thinking this same behavior with MySQL using (github.com/go-sql-driver/mysql)

Eventually i found unexpected behavior (or bug) of lib/pq

I filed an issue report on lib/pq.

Thank you @bcmills, @renthraysk for investigating.

@golang golang locked and limited conversation to collaborators Feb 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants