Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(819)

Issue 6855102: database/sql: expose DB.MaxIdleConns and add DB.SetMaxIdleConns (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 2 months ago by raggi
Modified:
5 years, 10 months ago
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r 616adac0bb59 https://code.google.com/p/go/ #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -9 lines) Patch
M src/pkg/database/sql/sql.go View 1 4 chunks +28 lines, -9 lines 6 comments Download
M src/pkg/database/sql/sql_test.go View 1 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 6
raggi
The purpose of this patch is to allow for significant improvements in concurrent database access ...
6 years, 2 months ago (2012-11-30 02:27:54 UTC) #1
raggi
6 years ago (2013-01-20 22:41:24 UTC) #2
raggi
The purpose of this patch is to allow for significant improvements in concurrent database access ...
6 years ago (2013-02-04 02:30:20 UTC) #3
raggi
database/sql: expose DB.MaxIdleConns and add DB.SetMaxIdleConns The purpose of this patch is to allow for ...
5 years, 11 months ago (2013-03-08 02:16:43 UTC) #4
bradfitz
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#newcode228 src/pkg/database/sql/sql.go:228: // Adjust the maximum number ...
5 years, 11 months ago (2013-03-08 22:40:08 UTC) #5
julienschmidt
5 years, 11 months ago (2013-03-09 02:10:02 UTC) #6
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b