code review 9789044: database/sql: Add SetMaxOpenConns()

database/sql: Add SetMaxOpenConns()

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.

[Tad Glines, 2013-05-26 17:12:06]

Regarding the change to DB.Close(): This was the one place where the conn's
finalClose() was being called while db.mu was locked. This is contrary to the
contract specified for the finalCloser interface. This wasn't a problem before
because the conn's finalClose() didn't need to touch the db. But, with the
addition of MaxOpenConns, it was necessary to fix this so that the conn's
finalClose() could decrement the numOpen count and signal db.cond.

[bradfitz, 2013-06-14 19:46:33]

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/fakedb_...
File src/pkg/database/sql/fakedb_test.go (right):

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/fakedb_...
src/pkg/database/sql/fakedb_test.go:503: case "NOSERT":
what is NOSERT? or execInsert's doInsert bool? Neither are documented.

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

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:196: cond     sync.Cond
I skeptical of any code that feels it necessary to use a sync.Cond.  It's
definitely not code I want to maintain and reason about in the future.

I believe Ian or somebody recently went so far as to say: never use sync.Cond. 
It's there for low-level packages.  I doubt it's required here.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:260: func (dc *driverConn) closeDBLocked() func()
error {
this function name now doesn't really match what it does.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:431: func (db *DB) Close() error {
why did this function change?

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:481: // MAke sure maxIdle doesn't exeed maxOpen
two typos on this line

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:495: // can maintain.
it doesn't change the capabilities of the DB server.

it affects how many connections this *DB makes to the server.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:504: var syncMaxIdle bool = false
no need to say bool. or var.

[Tad Glines, 2013-06-14 21:12:00]

Update to CL that addresses some comments to follow shortly.

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

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:196: cond     sync.Cond
On 2013/06/14 19:46:33, bradfitz wrote:
> I skeptical of any code that feels it necessary to use a sync.Cond.  It's
> definitely not code I want to maintain and reason about in the future.
> 
> I believe Ian or somebody recently went so far as to say: never use sync.Cond.

> It's there for low-level packages.  I doubt it's required here.

While sync.Cond isn't required, the solution using sync.Cond has fewer moving
parts (and is likely more performant) than the "request list/slice+channels+go
routine opener" alternative.

Can you elaborate on why you are skeptical of code that uses sync.Cond?

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:260: func (dc *driverConn) closeDBLocked() func()
error {
On 2013/06/14 19:46:33, bradfitz wrote:
> this function name now doesn't really match what it does.

The name still matches what it does. Look at line 436. It is called while db.mu
is locked.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:431: func (db *DB) Close() error {
On 2013/06/14 19:46:33, bradfitz wrote:
> why did this function change?

I explained the change in my first comment on this CL. To repeat:

This was the one place where the conn's
finalClose() was being called while db.mu was locked. This is contrary to the
contract specified for the finalCloser interface. This wasn't a problem before
because the conn's finalClose() didn't need to touch the db. But, with the
addition of MaxOpenConns, it was necessary to fix this so that the conn's
finalClose() could decrement the numOpen count and signal db.cond.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:481: // MAke sure maxIdle doesn't exeed maxOpen
On 2013/06/14 19:46:33, bradfitz wrote:
> two typos on this line

Done.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:495: // can maintain.
On 2013/06/14 19:46:33, bradfitz wrote:
> it doesn't change the capabilities of the DB server.
> 
> it affects how many connections this *DB makes to the server.

I'll change the wording.

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:504: var syncMaxIdle bool = false
On 2013/06/14 19:46:33, bradfitz wrote:
> no need to say bool. or var.

I realize now that I can just delete the entire line and just add a colon on
line 510.

[Tad Glines, 2013-06-14 21:13:27]

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

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

[Tad Glines, 2013-06-14 21:17:31]

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/fakedb_...
File src/pkg/database/sql/fakedb_test.go (right):

https://codereview.appspot.com/9789044/diff/2001/src/pkg/database/sql/fakedb_...
src/pkg/database/sql/fakedb_test.go:503: case "NOSERT":
On 2013/06/14 19:46:33, bradfitz wrote:
> what is NOSERT? or execInsert's doInsert bool? Neither are documented.

I added comments to address this in the patch I just uploaded, but forgot to
comment on this directly.

NOSERT does everything INSERT does except it doesn't add the row to the table. I
added it so that I could use it in the concurrent benchmarks. I wanted it to
excursive the same code paths in database/sql as a real INSERT but without the
having to worry about the added overhead of a ballooning table.

[ioe, 2013-06-14 23:02:32]

On 2013/06/14 21:17:31, Tad Glines wrote:
> NOSERT does everything INSERT does except it doesn't add the row to the table.
I
> added it so that I could use it in the concurrent benchmarks. I wanted it to
> excursive the same code paths in database/sql as a real INSERT but without the
> having to worry about the added overhead of a ballooning table.

What about calling it "UPDATE" instead? As an UPDATE has the same semantics.

[Tad Glines, 2013-06-14 23:49:48]

On 2013/06/14 23:02:32, ioe wrote:
> On 2013/06/14 21:17:31, Tad Glines wrote:
> > NOSERT does everything INSERT does except it doesn't add the row to the
table.
> I
> > added it so that I could use it in the concurrent benchmarks. I wanted it to
> > excursive the same code paths in database/sql as a real INSERT but without
the
> > having to worry about the added overhead of a ballooning table.
> 
> What about calling it "UPDATE" instead? As an UPDATE has the same semantics.

UPDATE isn't implemented in fakedb and NOSERT was easier to add than
implementing actual UPDATE functionality.
I could name it FAKE_INSERT instead of NOSERT if that would make it clearer.

[bradfitz, 2013-06-27 17:40:38]

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

https://codereview.appspot.com/9789044/diff/9001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:196: cond     sync.Cond
I didn't really don't want to see a sync.Cond in this code.  The usage in this
CL seems mostly fine (see comment below), but it adds complexity to code that is
arguably already borderline too complex.  I'd much prefer something using
higher-level idiomatic Go mechanisms.

That is, can we make this code simpler at the same time?

I'd fine introducing new (unexported) types and interfaces and spilling out into
a new file if necessary, if all the pieces are clear.

Or not going that far, what does this CL look like mostly as-is, but without the
sync.Cond?

Can we have a connection manager goroutine (run for the life of the life of the
*DB) that owns the pool and hands out connections and counts?

https://codereview.appspot.com/9789044/diff/9001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:469: // then the new MaxIdfleConns will be reduced
to match the MaxOpenConns limit
typo

https://codereview.appspot.com/9789044/diff/9001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:519: if db.closed {
doesn't this check need to be be done after re-acquiring the lock after the
cond.Wait?

https://codereview.appspot.com/9789044/diff/9001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:524: // If db.maxOpen > 0 then wait until either the
number of open connectsion
typo

[Tad Glines, 2013-06-27 18:16:07]

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

https://codereview.appspot.com/9789044/diff/9001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:196: cond     sync.Cond
On 2013/06/27 17:40:38, bradfitz wrote:
> I didn't really don't want to see a sync.Cond in this code.  The usage in this
> CL seems mostly fine (see comment below), but it adds complexity to code that
is
> arguably already borderline too complex.  I'd much prefer something using
> higher-level idiomatic Go mechanisms.
> 
> That is, can we make this code simpler at the same time?
> 
> I'd fine introducing new (unexported) types and interfaces and spilling out
into
> a new file if necessary, if all the pieces are clear.
> 
> Or not going that far, what does this CL look like mostly as-is, but without
the
> sync.Cond?
> 
> Can we have a connection manager goroutine (run for the life of the life of
the
> *DB) that owns the pool and hands out connections and counts?
> 

My original goals when writing this Cl where:
1. Minimize delta size (lines changed/added)
2. Zero (or nearly zero) impact to existing performance characteristics.

If neither of those are important, then yes, a channel base solution is
certainly possible. I did it once already, I can do it again :-)

I'll create a new CL with a channel based solution.