code review 12646044: database/sql: Close bad driver connections so they're r...

database/sql: Close bad driver connections so they're removed from the statement connections list. Fixes an issue where prepared statements that outlive many connections become expensive to invoke.

Fixes issue 6081

[anacrolix, 2013-08-09 06:01:26]

Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org),

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

[bradfitz, 2013-08-09 20:18:19]

All changes should include a test.

Can you describe what's happening here such that a test could be written (I
could help) to demonstrate this happening?



On Thu, Aug 8, 2013 at 11:01 PM, <anacrolix@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> database/sql: Close bad driver connections so they're removed from the
> statement connections list. Fixes an issue where prepared statements
> that outlive many connections become expensive to invoke.
>
> Fixes issue 6081
>
> Please review this at
https://codereview.appspot.**com/12646044/<https://codereview.appspot.com/126...
>
> Affected files:
>   M src/pkg/database/sql/sql.go
>
>
> Index: src/pkg/database/sql/sql.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql.go
> +++ b/src/pkg/database/sql/sql.go
> @@ -518,12 +518,12 @@
>  func (db *DB) connIfFree(wanted *driverConn) (*driverConn, error) {
>         db.mu.Lock()
>         defer db.mu.Unlock()
> +       if wanted.dbmuClosed {
> +               return nil, errConnClosed
> +       }
>         if wanted.inUse {
>                 return nil, errConnBusy
>         }
> -       if wanted.dbmuClosed {
> -               return nil, errConnClosed
> -       }
>         for i, conn := range db.freeConn {
>                 if conn != wanted {
>                         continue
> @@ -590,6 +590,7 @@
>         if err == driver.ErrBadConn {
>                 // Don't reuse bad connections.
>                 db.mu.Unlock()
> +               dc.Close()
>                 return
>         }
>         if putConnHook != nil {
>
>
>

[anacrolix, 2013-08-10 04:35:45]

In func (s *Stmt) connStmt(), an array of connStmt's, s.css, is traversed
to locate a connection that has previously prepared the current statement.
When calling connIfFree to determine if a connection is available,
ErrConnClosed is not returned as it should be for connections that were
discarded due to ErrBadConn in func (db *DB) putConn. In func (db *DB)
putConn, if a connection returns ErrBadConn, dc.Close is not called, and so
the variable dbmuClosed is not set to true as it should be. This means that
the connIfFree function in turn doesn't return ErrConnClosed, and so the
connection is never removed from arrays of preprepared connections on
Stmt's. This in turn means that as connections are discarded over time due
to ErrBadConn, large numbers of dead connections accumulate in Stmt.css,
and have to be walked through on every invocation of a Stmt via Query or
Exec. This costs a lot of time, and prepared statements become slower and
slower.


On Sat, Aug 10, 2013 at 6:18 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> All changes should include a test.
>
> Can you describe what's happening here such that a test could be written
> (I could help) to demonstrate this happening?
>
>
>
> On Thu, Aug 8, 2013 at 11:01 PM, <anacrolix@gmail.com> wrote:
>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> database/sql: Close bad driver connections so they're removed from the
>> statement connections list. Fixes an issue where prepared statements
>> that outlive many connections become expensive to invoke.
>>
>> Fixes issue 6081
>>
>> Please review this at
https://codereview.appspot.**com/12646044/<https://codereview.appspot.com/126...
>>
>> Affected files:
>>   M src/pkg/database/sql/sql.go
>>
>>
>> Index: src/pkg/database/sql/sql.go
>> ==============================**==============================**=======
>> --- a/src/pkg/database/sql/sql.go
>> +++ b/src/pkg/database/sql/sql.go
>> @@ -518,12 +518,12 @@
>>  func (db *DB) connIfFree(wanted *driverConn) (*driverConn, error) {
>>         db.mu.Lock()
>>         defer db.mu.Unlock()
>> +       if wanted.dbmuClosed {
>> +               return nil, errConnClosed
>> +       }
>>         if wanted.inUse {
>>                 return nil, errConnBusy
>>         }
>> -       if wanted.dbmuClosed {
>> -               return nil, errConnClosed
>> -       }
>>         for i, conn := range db.freeConn {
>>                 if conn != wanted {
>>                         continue
>> @@ -590,6 +590,7 @@
>>         if err == driver.ErrBadConn {
>>                 // Don't reuse bad connections.
>>                 db.mu.Unlock()
>> +               dc.Close()
>>                 return
>>         }
>>         if putConnHook != nil {
>>
>>
>>
>

[anacrolix, 2013-08-11 04:39:33]

Not sure if my previous message was received.

[anacrolix, 2013-08-12 11:13:07]

On production systems at Brandscreen we're seeing the Stmt.css array get as long
as 1850, introducing delays in calling SQL statements of more than 20ms.

Please let me know what is left to get this in, I believe it should not wait for
Go1.2 as it's a bug, not a change in behaviour or feature.

[bradfitz, 2013-08-12 15:09:41]

Go 1.2 will be the default, unless this gets in before a point release and
we cherry-pick it over to the point release branch.  But we only do that if
the bug is serious, the fix is obviously correct and minor, and there are
good tests.

There are no tests here yet.  I'll look into writing some, but maybe you
can beat me to it.



On Mon, Aug 12, 2013 at 4:13 AM, <anacrolix@gmail.com> wrote:

> On production systems at Brandscreen we're seeing the Stmt.css array get
> as long as 1850, introducing delays in calling SQL statements of more
> than 20ms.
>
> Please let me know what is left to get this in, I believe it should not
> wait for Go1.2 as it's a bug, not a change in behaviour or feature.
>
>
https://codereview.appspot.**com/12646044/<https://codereview.appspot.com/126...
>

[bradfitz, 2013-08-13 22:24:04]

LGTM

I can't submit this until you've submitted a CLA, though.

See http://golang.org/doc/contribute.html#copyright

Let me know when that's in.



On Thu, Aug 8, 2013 at 11:01 PM, <anacrolix@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> database/sql: Close bad driver connections so they're removed from the
> statement connections list. Fixes an issue where prepared statements
> that outlive many connections become expensive to invoke.
>
> Fixes issue 6081
>
> Please review this at
https://codereview.appspot.**com/12646044/<https://codereview.appspot.com/126...
>
> Affected files:
>   M src/pkg/database/sql/sql.go
>
>
> Index: src/pkg/database/sql/sql.go
> ==============================**==============================**=======
> --- a/src/pkg/database/sql/sql.go
> +++ b/src/pkg/database/sql/sql.go
> @@ -518,12 +518,12 @@
>  func (db *DB) connIfFree(wanted *driverConn) (*driverConn, error) {
>         db.mu.Lock()
>         defer db.mu.Unlock()
> +       if wanted.dbmuClosed {
> +               return nil, errConnClosed
> +       }
>         if wanted.inUse {
>                 return nil, errConnBusy
>         }
> -       if wanted.dbmuClosed {
> -               return nil, errConnClosed
> -       }
>         for i, conn := range db.freeConn {
>                 if conn != wanted {
>                         continue
> @@ -590,6 +590,7 @@
>         if err == driver.ErrBadConn {
>                 // Don't reuse bad connections.
>                 db.mu.Unlock()
> +               dc.Close()
>                 return
>         }
>         if putConnHook != nil {
>
>
>

[anacrolix, 2013-08-14 02:26:58]

The CLA is in. I've verified the test demonstrates the problem, and the patch
fixes it.

[anacrolix, 2013-08-14 02:34:12]

Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org,
golang-dev@googlegroups.com),

Please take another look.

[anacrolix, 2013-08-14 02:35:12]

Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org,
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-08-14 16:27:33]

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

database/sql: fix accumulation of bad conns on prepared statements

Fixes an issue where prepared statements that outlive many
connections become expensive to invoke.

Fixes issue 6081

R=golang-dev
CC=bradfitz, golang-dev
https://codereview.appspot.com/12646044

Committer: Brad Fitzpatrick <bradfitz@golang.org>