database/sql: expose DB.MaxIdleConns and add DB.SetMaxIdleConns

database/sql: expose DB.MaxIdleConns and add DB.SetMaxIdleConns

The purpose of this patch is to allow for significant improvements in concurrent database access using the standard database/sql pool. As evidenced by work derived from the Boundary "Go vs. Java" blog post, the current concurent performance is limited for certain use cases (such as net/http serving data from bmizerany/pq)[1]. Additional changes from the original blog post, including benchmarking using more modern tools is also available on GitHub[2], some results are included[3].

Additional notes:

 * The current code defaults to 2 "idle connections". In reality this equates to 2 "active connections" for various reasons. This default value is continued only such that users would not be subject to unexpected semantic changes.
 * There is no maximum limit implemented. While not the subject of this problem or patch, this should be implemented.
 * The immediate close behavior (something I am tempted to consider a bug) has not been altered. This point may become more of a philosophical discussion. In my experience, it's better to handle bursts with grace where possible. This would also relieve the scheduler.
 * It may make sense to move the mutex in database/sql to a RW variant with a persistent store of Conn objects and a free / use list (with a slow garbage collector). This would also simplify the Close anti-race logic.
 * The second comment on Open suggests that drivers will implement an Open that returns a *DB. This is no longer the case, and this comment should be removed.
 * The default value should be documented somewhere.

[1] http://boundary.com/blog/2012/09/17/comparing-go-and-java-part-2-performance/
[2] https://github.com/raggi/go-and-java
[3] https://github.com/raggi/go-and-java/blob/master/README.md#example-results

[raggi, 2012-11-30 02:27:54]

The purpose of this patch is to allow for significant improvements in concurrent
database access using the standard database/sql pool. As evidenced by work
derived from the Boundary "Go vs. Java" blog post, the current concurent
performance is limited for certain use cases (such as net/http serving data from
bmizerany/pq)[1]. Additional changes from the original blog post, including
benchmarking using more modern tools is also available on GitHub[2], some
results are included[3].

Additional notes:

 * The current code defaults to 2 "idle connections". In reality this equates to
2 "active connections" for various reasons. This default value is continued only
such that users would not be subject to unexpected semantic changes.
 * There is no maximum limit implemented. While not the subject of this problem
or patch, this should be implemented.
 * The immediate close behavior (something I am tempted to consider a bug) has
not been altered. This point may become more of a philosophical discussion. In
my experience, it's better to handle bursts with grace where possible. This
would also relieve the scheduler.
 * It may make sense to move the mutex in database/sql to a RW variant with a
persistent store of Conn objects and a free / use list (with a slow garbage
collector). This would also simplify the Close anti-race logic.
 * The second comment on Open suggests that drivers will implement an Open that
returns a *DB. This is no longer the case, and this comment should be removed.
 * The default value should be documented somewhere.

[1]
http://boundary.com/blog/2012/09/17/comparing-go-and-java-part-2-performance/
[2] https://github.com/raggi/go-and-java
[3] https://github.com/raggi/go-and-java/blob/master/README.md#example-results

[raggi, 2013-02-04 02:30:20]

The purpose of this patch is to allow for significant improvements in concurrent
database access using the standard database/sql pool. As evidenced by work
derived from the Boundary "Go vs. Java" blog post, the current concurent
performance is limited for certain use cases (such as net/http serving data from
bmizerany/pq)[1]. Additional changes from the original blog post, including
benchmarking using more modern tools is also available on GitHub[2], some
results are included[3].

Additional notes:

 * The current code defaults to 2 "idle connections". In reality this equates to
2 "active connections" for various reasons. This default value is continued only
such that users would not be subject to unexpected semantic changes.
 * There is no maximum limit implemented. While not the subject of this problem
or patch, this should be implemented.
 * The immediate close behavior (something I am tempted to consider a bug) has
not been altered. This point may become more of a philosophical discussion. In
my experience, it's better to handle bursts with grace where possible. This
would also relieve the scheduler.
 * It may make sense to move the mutex in database/sql to a RW variant with a
persistent store of Conn objects and a free / use list (with a slow garbage
collector). This would also simplify the Close anti-race logic.
 * The second comment on Open suggests that drivers will implement an Open that
returns a *DB. This is no longer the case, and this comment should be removed.
 * The default value should be documented somewhere.

[1]
http://boundary.com/blog/2012/09/17/comparing-go-and-java-part-2-performance/
[2] https://github.com/raggi/go-and-java
[3] https://github.com/raggi/go-and-java/blob/master/README.md#example-results

[raggi, 2013-03-08 02:16:43]

database/sql: expose DB.MaxIdleConns and add DB.SetMaxIdleConns

The purpose of this patch is to allow for significant improvements in concurrent
database access using the standard database/sql pool. As evidenced by work
derived from the Boundary "Go vs. Java" blog post, the current concurent
performance is limited for certain use cases (such as net/http serving data from
bmizerany/pq)[1]. Additional changes from the original blog post, including
benchmarking using more modern tools is also available on GitHub[2], some
results are included[3].

Additional notes:

 * The current code defaults to 2 "idle connections". In reality this equates to
2 "active connections" for various reasons. This default value is continued only
such that users would not be subject to unexpected semantic changes.
 * There is no maximum limit implemented. While not the subject of this problem
or patch, this should be implemented.
 * The immediate close behavior (something I am tempted to consider a bug) has
not been altered. This point may become more of a philosophical discussion. In
my experience, it's better to handle bursts with grace where possible. This
would also relieve the scheduler.
 * It may make sense to move the mutex in database/sql to a RW variant with a
persistent store of Conn objects and a free / use list (with a slow garbage
collector). This would also simplify the Close anti-race logic.
 * The second comment on Open suggests that drivers will implement an Open that
returns a *DB. This is no longer the case, and this comment should be removed.
 * The default value should be documented somewhere.

[1]
http://boundary.com/blog/2012/09/17/comparing-go-and-java-part-2-performance/
[2] https://github.com/raggi/go-and-java
[3] https://github.com/raggi/go-and-java/blob/master/README.md#example-results

[bradfitz, 2013-03-08 22:40:08]

comments and thoughts ...

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

https://codereview.appspot.com/6855102/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:228: // Adjust the maximum number of permanently
retained database connections. This
See next comment.

// SetMaxIdleConns sets ...

https://codereview.appspot.com/6855102/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:229: // defaults to 2 for historical reasons. N.B.
This is not the maximum number of
I think we should instead make an exported constant for 2 (DefaultMaxIdleConns)
and reference that.  No need to say "for historical reasons".  Drop the "N.B.",
as good as the note is.  We avoid e.g. i.e. n.b.

https://codereview.appspot.com/6855102/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:239: // The current maximum number of idle database
connections allowed. See
Go comment style is to start sentences with the name of the thing being
described. They have to stand alone.

// MaxIdleConns returns the ....

https://codereview.appspot.com/6855102/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:242: // TODO(bradfitz): ask driver, if supported,
for its default preference
probably not here anymore.  Probably only once the driver is opened (where you
have the 2 now).

you should probably have a func like:

func initialMaxIdleConns(db *DB) int {
   if di, ok := db.driver.(driver.MaxIdleConner); ok {
        if v, ok := di.MaxIdleConns(db.dsn); ok {
            return v
        }
   }
   return DefaultMaxIdleConns
}

and then call that from Open.

But rather than a proliferation of small preferred-config interfaces like that
in the driver package, perhaps we should just have an alternate Open interface
that returns that config from the dsn once.

Imagine, in the driver package:

type DriverConfig struct [
   MaxIdleConns int

   // future stuff

   NewConn func() (Conn, Error)
}

And then we add:

package driver
type DriverConfig interface {
    OpenConfig(dsn string) (*DriverConfig, error)
}

And we prefer to use that if it exists, else falling back to driver.Driver's
Open instead.

That seems more extensible.

Thoughts?

https://codereview.appspot.com/6855102/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:293: maxIdle := db.MaxIdleConns()
this is grabbing a lock and unlocking it, even though you grab the same lock on
the next line.

You can just use db.maxIdleConns directly once you're holding the lock.

[julienschmidt, 2013-03-09 02:10:02]

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

https://codereview.appspot.com/6855102/diff/2001/src/pkg/database/sql/sql.go#...
src/pkg/database/sql/sql.go:242: // TODO(bradfitz): ask driver, if supported,
for its default preference
I like the idea of a config struct ( like I did here
https://codereview.appspot.com/7365049/ ;) ) but I don't think we need 3 Open
functions. It might be a good idea to combine this with
http://code.google.com/p/go/issues/detail?id=4804 .
The "NewConn" function would also go well together with the "clone" function
(driver side) I suggested in the last comment.

If you agree, I try to sent out a CL for the new Open function ASAP.

On 2013/03/08 22:40:08, bradfitz wrote:
> But rather than a proliferation of small preferred-config interfaces like that
> in the driver package, perhaps we should just have an alternate Open interface
> that returns that config from the dsn once.
> 
> Imagine, in the driver package:
> 
> type DriverConfig struct [
>    MaxIdleConns int
> 
>    // future stuff
> 
>    NewConn func() (Conn, Error)
> }
> 
> And then we add:
> 
> package driver
> type DriverConfig interface {
>     OpenConfig(dsn string) (*DriverConfig, error)
> }
> 
> And we prefer to use that if it exists, else falling back to driver.Driver's
> Open instead.
> 
> That seems more extensible.
> 
> Thoughts?