code review 12480043: database/sql: doc clarifications on Rows.Close and Err

database/sql: doc clarifications on Rows.Close and Err

[bradfitz, 2013-08-05 20:57:14]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[nigeltao, 2013-08-05 23:32:46]

https://codereview.appspot.com/12480043/diff/6001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/12480043/diff/6001/src/pkg/database/sql/sql.go...
src/pkg/database/sql/sql.go:1377: // end is encountered, the Rows are closed
automatically. Close
What does "the end is encountered" mean exactly? Is it the same as "I called
Next and it returned false"? Or do I also need to check the return value of Err?

Looking at the source code for Rows.Next, if lasterr is non-nil but also non-EOF
then Next will return false but Close will not be implicitly called. Is that
intentional? Does the example code that says

for rows.Next()
  etc
}
err = rows.Err() // get any error encountered during iteration

need to add:

if err != nil {
  rows.Close()
}

[nigeltao, 2013-08-07 00:41:20]

Ping.

[bradfitz, 2013-08-07 00:54:17]

I didn't know the answer off-hand.

I don't think it's defined.

Do you think it should be?  If so, how?

I don't think I want users to have to call Close if there's an error.

I think if rows.Next() ever returns false (for EOF or other reason), rows
should be automatically closed.  I don't know whether that's the case
today.  You want to check/fix/document?




On Tue, Aug 6, 2013 at 5:41 PM, <nigeltao@golang.org> wrote:

> Ping.
>
>
https://codereview.appspot.**com/12480043/<https://codereview.appspot.com/124...
>

[nigeltao, 2013-08-07 01:53:05]

On Wed, Aug 7, 2013 at 10:54 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I think if rows.Next() ever returns false (for EOF or other reason), rows
> should be automatically closed.  I don't know whether that's the case today.
> You want to check/fix/document?

That's what I expected too, but that's not what currently happens.
Rows.Next ends with:

rs.lasterr = rs.rowsi.Next(rs.lastcols)
if rs.lasterr == io.EOF {
  rs.Close()
}
return rs.lasterr == nil

So rs.Close might not be called even if Rows returns false. The patch
might be as simple as s/== io.EOF/!= nil/ but I don't know if this
will break anything. You know the database/sql package better than I
do, and you already have this CL open. Can I punt this back to you?

[nigeltao, 2013-08-07 01:54:53]

On Wed, Aug 7, 2013 at 11:53 AM, Nigel Tao <nigeltao@golang.org> wrote:
> rs.lasterr = rs.rowsi.Next(rs.lastcols)
> if rs.lasterr == io.EOF {
>   rs.Close()
> }
> return rs.lasterr == nil
>
> So rs.Close might not be called even if Rows returns false.

I meant:
So rs.Close might not be called even if Next returns false

[nigeltao, 2013-08-12 01:37:56]

Ping.

[nigeltao, 2013-08-12 07:05:16]

Pleas review https://codereview.appspot.com/12677050 instead.

[gobot, 2013-12-20 16:25:59]

Replacing golang-dev with golang-codereviews.

[bradfitz, 2013-12-20 18:03:19]

R=close

Since clarified in docs in another CL.