code review 14920046: database/sql: fix prepared statements Exec / Query auto...

database/sql: fix auto-reconnect in prepared statements

This also fixes several connection leaks.
Fixes issue 5718

[julienschmidt, 2013-10-20 06:03:30]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
alberto.garcia.hierro@gmail.com, golang-dev@googlegroups.com,
tad.glines@gmail.com),

I'd like you to review this change to
https://code.google.com/p/go

[Tad Glines, 2013-10-20 17:48:08]

On a philosophical note, I've never been particularly happy with the way bad
connections are handled. With a fixed loop constant of 10, if the database is
restarted and there where more than 10 connection in the idle pool, the Query or
Exec will still fail.

I've submitted a separate CL that incorporates and builds on this one. It
addresses two additional issue that this Cl does not. One, it works regardless
of the MaxIdle value. Two, it addresses the comments I made to this CL.

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

https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1253: if err != nil {
connStmt will try and prepare the statement and can return driver.ErrBaddConn.
So only return if err != driver.ErrBadConn, otherwise continue.

https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1376: if err != nil {
Same comment as for stmt.Exec

[Tad Glines, 2013-10-20 17:49:18]

The alternative CL is https://codereview.appspot.com/15400044

On 2013/10/20 17:48:08, Tad Glines wrote:
> On a philosophical note, I've never been particularly happy with the way bad
> connections are handled. With a fixed loop constant of 10, if the database is
> restarted and there where more than 10 connection in the idle pool, the Query
or
> Exec will still fail.
> 
> I've submitted a separate CL that incorporates and builds on this one. It
> addresses two additional issue that this Cl does not. One, it works regardless
> of the MaxIdle value. Two, it addresses the comments I made to this CL.
> 
>
https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql.go
> File src/pkg/database/sql/sql.go (right):
> 
>
https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1253: if err != nil {
> connStmt will try and prepare the statement and can return driver.ErrBaddConn.
> So only return if err != driver.ErrBadConn, otherwise continue.
> 
>
https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1376: if err != nil {
> Same comment as for stmt.Exec

[julienschmidt, 2013-10-20 17:52:07]

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

https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1253: if err != nil {
On 2013/10/20 17:48:08, Tad Glines wrote:
> connStmt will try and prepare the statement and can return driver.ErrBaddConn.
> So only return if err != driver.ErrBadConn, otherwise continue.

connStmt has its own retry-loop. It only returns driver.ErrBadConn if 10 tries
are exceeded.

[bradfitz, 2013-10-20 17:56:14]

This is too late for Go 1.2.  This should've been brought up weeks or
months ago.



On Sun, Oct 20, 2013 at 10:48 AM, <Tad.Glines@gmail.com> wrote:

> On a philosophical note, I've never been particularly happy with the way
> bad connections are handled. With a fixed loop constant of 10, if the
> database is restarted and there where more than 10 connection in the
> idle pool, the Query or Exec will still fail.
>
> I've submitted a separate CL that incorporates and builds on this one.
> It addresses two additional issue that this Cl does not. One, it works
> regardless of the MaxIdle value. Two, it addresses the comments I made
> to this CL.
>
>
> https://codereview.appspot.**com/14920046/diff/120001/src/**
>
pkg/database/sql/sql.go<https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql.go>
> File src/pkg/database/sql/sql.go (right):
>
> https://codereview.appspot.**com/14920046/diff/120001/src/**
>
pkg/database/sql/sql.go#**newcode1253<https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql.go#newcode1253>
> src/pkg/database/sql/sql.go:**1253: if err != nil {
> connStmt will try and prepare the statement and can return
> driver.ErrBaddConn. So only return if err != driver.ErrBadConn,
> otherwise continue.
>
> https://codereview.appspot.**com/14920046/diff/120001/src/**
>
pkg/database/sql/sql.go#**newcode1376<https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql.go#newcode1376>
> src/pkg/database/sql/sql.go:**1376: if err != nil {
> Same comment as for stmt.Exec
>
>
https://codereview.appspot.**com/14920046/<https://codereview.appspot.com/149...
>

[Tad Glines, 2013-10-20 18:00:05]

On 2013/10/20 17:52:07, julienschmidt wrote:
>
https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql.go
> File src/pkg/database/sql/sql.go (right):
> 
>
https://codereview.appspot.com/14920046/diff/120001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1253: if err != nil {
> On 2013/10/20 17:48:08, Tad Glines wrote:
> > connStmt will try and prepare the statement and can return
driver.ErrBaddConn.
> > So only return if err != driver.ErrBadConn, otherwise continue.
> 
> connStmt has its own retry-loop. It only returns driver.ErrBadConn if 10 tries
> are exceeded.

Sorry, yes, in your version that's true.
In my version I removed the retry loop from connStmt because Exec and Query
already have retry loops.
If both Exec/Query and connStmt have retry loops, then it is possible for Exec
or Query to end up making close to 100 reconnect attempts.

[julienschmidt, 2013-10-20 18:58:03]

On 2013/10/20 17:49:18, Tad Glines wrote:
> The alternative CL is https://codereview.appspot.com/15400044

I'd suggest to make a follow-up CL.
The purpose of this CL is to fix things, while your CL would add a behavior
change.

On 2013/10/20 17:56:14, bradfitz wrote:
> This is too late for Go 1.2.  This should've been brought up weeks or
> months ago.

I'm surprised no one really looked into this issue before. I had no time for it
during the last months, but I in my opinion a Go version labeled as stable
shouldn't be released with a known flaw like this. This is a bug fix, no more,
no less.

On 2013/10/20 18:00:05, Tad Glines wrote:
> If both Exec/Query and connStmt have retry loops, then it is possible for Exec
> or Query to end up making close to 100 reconnect attempts.

Fixed

[Tad Glines, 2013-10-20 19:22:33]

On 2013/10/20 18:58:03, julienschmidt wrote:
> On 2013/10/20 17:49:18, Tad Glines wrote:
> > The alternative CL is https://codereview.appspot.com/15400044
> 
> I'd suggest to make a follow-up CL.

By follow-up CL do you mean a CL that is a delta from this CL? Or do you mean,
wait for this CL be committed or closed and them submit a CL?

> The purpose of this CL is to fix things, while your CL would add a behavior
> change.

This CL extends the retry logic to cover all the other applicable cases that
where not covered before. This is good. However, the retry logic currently uses
a hard coded loop count of 10. I am suggesting that instead of using a hard
coded loop count of 10, the value (db.freeConn.Len() + 10) be used. This will
ensure that the retry logic works regardless of the number of idle connections.

I created a separate CL so I could include a Test that covers the case where
MaxIdle >= 10.

[Tad Glines, 2013-10-20 19:26:37]

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

https://codereview.appspot.com/14920046/diff/190001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1351: if err != nil {
The connection needs to be released or it will leak

if err == driver.ErrBadConn {
	// The conn is bad
	// It must be released or the connection will leak.
	s.db.putConn(dc, err)
}

[julienschmidt, 2013-10-20 20:12:52]

On 2013/10/20 19:22:33, Tad Glines wrote:
> On 2013/10/20 18:58:03, julienschmidt wrote:
> > On 2013/10/20 17:49:18, Tad Glines wrote:
> > > The alternative CL is https://codereview.appspot.com/15400044
> > 
> > I'd suggest to make a follow-up CL.
> 
> By follow-up CL do you mean a CL that is a delta from this CL? Or do you mean,
> wait for this CL be committed or closed and them submit a CL?

I think this CLs would overlap, so waiting for this CL to be committed / closed
would be the only option.

[julienschmidt, 2013-10-20 20:13:09]

On 2013/10/20 19:26:37, Tad Glines wrote:
>
https://codereview.appspot.com/14920046/diff/190001/src/pkg/database/sql/sql.go
> File src/pkg/database/sql/sql.go (right):
> 
>
https://codereview.appspot.com/14920046/diff/190001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1351: if err != nil {
> The connection needs to be released or it will leak
> 
> if err == driver.ErrBadConn {
> 	// The conn is bad
> 	// It must be released or the connection will leak.
> 	s.db.putConn(dc, err)
> }

Indeed. I uploaded a new revision which also includes a test for this.
I moved this into connStmt() to avoid complications with nil-pointers. There is
just a single case where we even have a connection to close.

This leak already exists in the current code, so this is another bug this CL
would fix.

[julienschmidt, 2013-10-20 20:15:54]

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

https://codereview.appspot.com/14920046/diff/190001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1351: if err != nil {
On 2013/10/20 19:26:37, Tad Glines wrote:
> The connection needs to be released or it will leak
> 
> if err == driver.ErrBadConn {
> 	// The conn is bad
> 	// It must be released or the connection will leak.
> 	s.db.putConn(dc, err)
> }

Oh wait, this IS in connStmt(). Nevermind :)
But I think the connection should be released regardless of the error being
driver.ErrBadConn or not.

[Tad Glines, 2013-10-20 21:17:09]

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

https://codereview.appspot.com/14920046/diff/190001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1351: if err != nil {
On 2013/10/20 20:15:54, julienschmidt wrote:
> On 2013/10/20 19:26:37, Tad Glines wrote:
> > The connection needs to be released or it will leak
> > 
> > if err == driver.ErrBadConn {
> > 	// The conn is bad
> > 	// It must be released or the connection will leak.
> > 	s.db.putConn(dc, err)
> > }
> 
> Oh wait, this IS in connStmt(). Nevermind :)
> But I think the connection should be released regardless of the error being
> driver.ErrBadConn or not.

Yes, that makes more sense. I'm not sure why I thought it only mattered if the
error was ErrBadConn.

[julienschmidt, 2013-10-20 22:21:50]

Unless you find another thing, I made one last update:
The test got very long, I tried to remove as much boilerplate code as possible.
Moreover the tests with the error occurring during Prepare in operations on
prepared statements should now be reliable.

[Tad Glines, 2013-10-20 23:08:23]

LGTM

Once this makes it in, I'll submit another CL (and issue) that deals with case
where MaxIdle >= 10.

[adg, 2013-10-21 04:28:33]

On 21 October 2013 03:58, <google@julienschmidt.com> wrote:

> I'm surprised no one really looked into this issue before. I had no time
> for it during the last months, but I in my opinion a Go version labeled
> as stable shouldn't be released with a known flaw like this. This is a
> bug fix, no more, no less.
>

There are more than 500 open, accepted
issues<https://code.google.com/p/go/issues/list?can=2&q=status%3AAccepted+&colspec=ID+Status+Stars+Priority+Owner+Reporter+Summary&cells=tiles>,
but we're still making a release.

You have to draw the line somewhere.

[julienschmidt, 2013-12-17 11:19:08]

ping

I this CL is blocking further CLs.

[julienschmidt, 2013-12-17 11:19:35]

> I this CL is blocking further CLs.
   ^ think

[bradfitz, 2013-12-17 19:57:18]

LGTM

[bradfitz, 2013-12-17 19:57:32]

*** Submitted as https://code.google.com/p/go/source/detail?r=3662d56e2402 ***

database/sql: fix auto-reconnect in prepared statements

This also fixes several connection leaks.
Fixes issue 5718

R=bradfitz, adg
CC=alberto.garcia.hierro, golang-dev
https://codereview.appspot.com/14920046

Committer: Brad Fitzpatrick <bradfitz@golang.org>