Re: database/sql: Add SetMaxOpenConns(), the chan based alt... (issue 10726044)

[brad...@golang.org]

LGTM

I've read this over again and am reasonably content maintaining it.
Enough people have expressed need for this, so I think it's worth the
cost of the additional complexity, which becomes less complex the more
I've read it.

And it has nice tests.

My apologies for the comically terrible review duration.


https://codereview.appspot.com/10726044/

[brad...@golang.org]

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

database/sql: add SetMaxOpenConns

Update issue 4805

Add the ability to set an open connection limit.
Fixed case where the Conn finalCloser was being called with db.mu
locked.
Added seperate benchmarks for each path for Exec and Query.
Replaced slice based idle pool with list based idle pool.

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

Committer: Brad Fitzpatrick <brad...@golang.org>


https://codereview.appspot.com/10726044/

[Brad Fitzpatrick]

But it breaks the build, so reverting.

[Brad Fitzpatrick]

Tad:

See http://build.golang.org/log/f53bbf9c888a0fe2c4a26d856c5e1449f055d287

Could you fix that?

The other failure is easier to fix:

deps_test.go needs an edge from database/sql to container/list, which I believe is acceptable.

[Tad.G...@gmail.com]

On 2013/08/30 00:27:57, bradfitz wrote:
> Tad:

> See
http://build.golang.org/log/f53bbf9c888a0fe2c4a26d856c5e1449f055d287

> Could you fix that?

> The other failure is easier to fix:

> deps_test.go needs an edge from database/sql to container/list, which
I
> believe is acceptable.




> On Thu, Aug 29, 2013 at 5:26 PM, Brad Fitzpatrick
<brad...@golang.org>wrote:

> > But it breaks the build, so reverting.
> >
> >
> > On Thu, Aug 29, 2013 at 5:20 PM, <mailto:brad...@golang.org> wrote:
> >
> >> *** Submitted as
> >>

https://code.google.com/p/go/**source/detail?r=c9bea548fb6f%3Chttps://code.google.com/p/go/source/detail?r=c9bea548fb6f%3E***

> >>
> >> database/sql: add SetMaxOpenConns
> >>
> >> Update issue 4805
> >>
> >> Add the ability to set an open connection limit.
> >> Fixed case where the Conn finalCloser was being called with db.mu
> >> locked.
> >> Added seperate benchmarks for each path for Exec and Query.
> >> Replaced slice based idle pool with list based idle pool.
> >>
> >> R=bradfitz
> >> CC=golang-dev
> >>

https://codereview.appspot.**com/10726044%3Chttps://codereview.appspot.com/10726044>
> >>
> >> Committer: Brad Fitzpatrick <mailto:brad...@golang.org>
> >>
> >>
> >>

https://codereview.appspot.**com/10726044/%3Chttps://codereview.appspot.com/10726044/>
> >>
> >
> >

Added container/list as a dependency to database/sql in deps_test.go.
Fixed the race in Stmt.finalCloser. If called via Stmt.Close Stmt.mu was
locked, if called from via Rows.Close Stmt.mu was not locked. Resolved
by locking Stmt.mu in Stmt.finalCloser and unlocking Stmt.mu in
Stmt.Close just before calling removeDeps.

I signed the CLA, so if possible, please commit the CL with author set
to "Tad Glines <tad.g...@gmail.com".


https://codereview.appspot.com/10726044/

[brad...@golang.org]

https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_test.go
File src/pkg/database/sql/sql_test.go (right):

https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_test.go#newcode1222
src/pkg/database/sql/sql_test.go:1222: fini(t testing.TB)
fini isn't a word or common abbreviation Could you rename this to
finish() (perhaps with a comment) like I did before?

Also, update the commit description to match how I renamed it in
https://code.google.com/p/go/source/detail?r=c9bea548fb6ff253455f52ba4e844094544181c9

Lower case "add", and remove "the chan based alternative" from the
subject.

Thanks!

https://codereview.appspot.com/10726044/

[Tad.G...@gmail.com]

https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_test.go
File src/pkg/database/sql/sql_test.go (right):

https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_test.go#newcode1222
src/pkg/database/sql/sql_test.go:1222: fini(t testing.TB)

On 2013/08/30 02:47:52, bradfitz wrote:
> fini isn't a word or common abbreviation Could you rename this to
finish()
> (perhaps with a comment) like I did before?

Done

https://codereview.appspot.com/10726044/

[Tad.G...@gmail.com]

On 2013/08/30 02:39:40, Tad Glines wrote:
> On 2013/08/30 00:27:57, bradfitz wrote:
> > Tad:
> >

> I signed the CLA, so if possible, please commit the CL with author set
to "Tad
> Glines <tad.g...@gmail.com".

I clearly didn't look at the commit log close enough. I see that you
already knew about the CLA and used the right author value.


https://codereview.appspot.com/10726044/

[Brad Fitzpatrick]

The codereview tool handles the Author stuff.

Btw, drop the parens from your subject.  See https://code.google.com/p/go/source/detail?r=c9bea548fb6ff253455f52ba4e844094544181c9 ... no () at the end.