code review 116930043: database/sql: Avoid re-preparing statements when all co...

database/sql: Avoid re-preparing statements when all connections are busy

Previously, if all connections were busy, we would always
re-prepare the statement on the connection we were assigned from
the pool.  That meant that if all connections were busy most of the
time, the number of prepared statements for each connection would
keep increasing over time.

Instead, after getting a free connection, check to see if the
statement has already been prepared on it, and reuse the statement
handle if so.

[johto, 2014-07-19 11:28:24]

Hello golang-codereviews@googlegroups.com,

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

[johto, 2014-07-20 12:22:48]

I've updated this patch set to improve an existing comment and to 
simplify the changes necessary to TestErrBadConnReconnect.


.marko

[gobot, 2014-08-03 17:21:25]

R=bradfitz@golang.org (assigned by r@golang.org)

[johto, 2014-08-16 11:53:25]

Ping?

[bradfitz, 2014-08-18 17:59:58]

You touched the test file but it doesn't look like there's necessarily a new
test, is there?

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

https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1360: // TODO(bradfitz): or wait always wait for
one? make configurable later?
the TODO was more readable before. What was "or wait always wait" mean?

https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1361: if !match {
maybe we invert this and return up here if match and then we can unindent this. 
or break this up into another func?  this whole thing is getting pretty long and
indenty.

https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1369: found := false
can't you reuse the match variable which is already false?

https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1395: conn := cs.dc
these two lines could go into a func which takes cs and returns the 4 things
below. then you can use it the 2 or 3 places above.

[johto, 2014-08-18 18:44:15]

On 2014-08-18 7:59 PM, bradfitz@golang.org wrote:
> You touched the test file but it doesn't look like there's necessarily a
> new test, is there?

No, but a previous test was failing because it was relying on an 
internal detail which this CL changes.

>
https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1360: // TODO(bradfitz): or wait always wait
> for one? make configurable later?
> the TODO was more readable before.

Perhaps, but it was also inaccurate.

> What was "or wait always wait" mean?

"or always wait".

>
https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1369: found := false
> can't you reuse the match variable which is already false?

*shrug* I guess.

>
https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1361: if !match {
> maybe we invert this and return up here if match and then we can
> unindent this.  or break this up into another func?  this whole thing is
> getting pretty long and indenty.
>
> 
https://codereview.appspot.com/116930043/diff/60001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1395: conn := cs.dc
> these two lines could go into a func which takes cs and returns the 4
> things below. then you can use it the 2 or 3 places above.

I could try and see what that would look like.


.marko

[bradfitz, 2014-08-28 15:27:14]

Status on this? The tree closes in 3 days.

[johto, 2014-08-28 15:47:27]

On 2014-08-28 5:27 PM, bradfitz@golang.org wrote:
> Status on this? The tree closes in 3 days.

Oh, I wasn't aware of that.  I've been on vacation (and still am, 
technically).  I'll post an updated version later today.


.marko

[johto, 2014-08-29 00:31:59]

On 8/28/14, 5:47 PM, I wrote:
> On 2014-08-28 5:27 PM, bradfitz@golang.org wrote:
>> Status on this? The tree closes in 3 days.
>
> I'll post an updated version later today.

I've uploaded a new version; please take another look.  I felt like 
using goto here was the cleanest way of writing this.  I couldn't come 
up with even anything even remotely reasonable for the name of the 
function to put the last two lines of code in, but that would have 
essentially been the same thing logic-wise anyway.

Thoughts?


.marko

[bradfitz, 2014-08-30 17:38:37]

On Thu, Aug 28, 2014 at 5:32 PM, Marko Tiikkaja <marko@joh.to> wrote:

> On 8/28/14, 5:47 PM, I wrote:
>
>> On 2014-08-28 5:27 PM, bradfitz@golang.org wrote:
>>
>>> Status on this? The tree closes in 3 days.
>>>
>>
>> I'll post an updated version later today.
>>
>
> I've uploaded a new version; please take another look.  I felt like using
> goto here was the cleanest way of writing this.  I couldn't come up with
> even anything even remotely reasonable for the name of the function to put
> the last two lines of code in, but that would have essentially been the
> same thing logic-wise anyway.
>
> Thoughts?


It's so easy to avoid goto here.

Just make a little function that takes a cs (or whatever) that returns the
4 things needed, including the always-nil error.

[johto, 2014-08-30 20:56:00]

On 8/30/14 7:38 PM, Brad Fitzpatrick wrote:
> It's so easy to avoid goto here.
>
> Just make a little function that takes a cs (or whatever) that returns the
> 4 things needed, including the always-nil error.

I don't think this is an improvement, but it's your call.  I've updated 
the CL.  Does the code look better to you?


.marko

[bradfitz, 2014-09-02 16:08:06]

LGTM

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

https://codereview.appspot.com/116930043/diff/100001/src/pkg/database/sql/sql...
src/pkg/database/sql/sql.go:1329: connStmtInfo := func(cs connStmt) (ci
*driverConn, releaseConn func(error), si driver.Stmt, err error) {
This could be a top-level function, but better I realize is to just ditch it and
inline it. Easier to understand callers that way anyway.

I'll do this before I submit to not drag this on longer.

[bradfitz, 2014-09-02 16:08:49]

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

database/sql: Avoid re-preparing statements when all connections are busy

Previously, if all connections were busy, we would always
re-prepare the statement on the connection we were assigned from
the pool.  That meant that if all connections were busy most of the
time, the number of prepared statements for each connection would
keep increasing over time.

Instead, after getting a free connection, check to see if the
statement has already been prepared on it, and reuse the statement
handle if so.

LGTM=bradfitz
R=golang-codereviews, gobot, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/116930043

Committer: Brad Fitzpatrick <bradfitz@golang.org>