|
|
|
database/sql: move pooling logic into seperate plugable interface, added ability to chain drivers.
Added the following interfaces:
ConnChecker
ChainableDriver
ChainedDriver
PoolableDriver
Pool
PooledStmt
Add new function OpenChain that allows creation of a driver chain and/or configuration of the default pool.
Default PoolableDriver's Pool implementation includes the following
optional capabilities:
- Limit number of connections
- Maintain a minimum number of connections
- Opening new connections in mulitples
- Close idle connections after a specified timeout (default is immediately)
- Check the connection before returning it from the pool.
- Check all idle connections periodically
- Use a custom query to check the connection (instead of the ConnChecker interface)
- Use the new ErrDBLimitExceeded (if returned by driver) to scale back the connection limit
- Configure the number of times to try acquiring a new connection before considering the DB dead and stoping all connection attempts
- Configure the delay before retrying a failed connection attempt.
- Configure an acquisition timeout. If set, the pool will return ErrOpenTimeout if it could not return a connection from the pool before the timeout.
Patch Set 1 #Patch Set 2 : diff -r 5e23075b0970 https://code.google.com/p/go #Patch Set 3 : diff -r 5e23075b0970 https://code.google.com/p/go #
Total comments: 6
Patch Set 4 : diff -r 5e23075b0970 https://code.google.com/p/go #
Total comments: 10
Patch Set 5 : diff -r 5e23075b0970 https://code.google.com/p/go #Patch Set 6 : diff -r 30c566874b83 https://code.google.com/p/go #Patch Set 7 : diff -r 30c566874b83 https://code.google.com/p/go #Patch Set 8 : diff -r d29da2ced72b https://code.google.com/p/go #Patch Set 9 : diff -r d29da2ced72b https://code.google.com/p/go #
Total messages: 23
|
Tad Glines
I just ran "go test -bench ." and there are some concurrency issues I was ...
|
12 years, 8 months ago (2013年04月27日 00:33:07 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I just ran "go test -bench ." and there are some concurrency issues I was not aware of. Once I track them down, I'll update this CL.
changeset 3 fixed the concurrency issue. Also, related to this CL as a whole: This change adds two dependencies to database/sql that didn't exist before: container/list encoding/json Right now the user has the option of passing in the Pool or ChainableDriver config either as the actual type used by the driver or as a JSON encoding of the data. The JSON encoding support isn't strictly necessary so I'm fine removing it is that needed. Regarding lists, I use them a lot and would prefer to depend on the container/list impl. I can implement my own internal to database/sql if necessary.
On 2013年04月27日 01:34:27, Tad Glines wrote: I cannot recall this CL being discussed at the mailing list _prior_ to entering the review process[1]. Also, it seems to ignore [2]. I suggest to reject the CL. [1]: http://golang.org/doc/contribute.html#Design [2]: http://golang.org/doc/go1compat.html
I have not reviewed your complete code yet, these are just a few first thoughts. Pool should be a sub-package like Driver if possible. Is the concept of chainable Drivers really necessary? I find it pretty confusing. https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go File src/pkg/database/sql/pool.go (right): https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... src/pkg/database/sql/pool.go:80: func (c *PoolConfig) fixValues() { return an error / panic instead of silently fixing the config? https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... src/pkg/database/sql/pool.go:149: } else if c, ok := config.(string); ok { I think this part should be removed. One way to configure the pool is enough. https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (right): https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/sql.go#... src/pkg/database/sql/sql.go:213: func Open(driverName, dataSourceName interface{}) (*DB, error) { I'm not sure if this breaks the API contract but I don't think this is a good idea anyways. It might be better to have a separate OpenChain(...) function. This avoids much confusion and possible errors.
On 2013年04月27日 07:58:14, 0xjnml wrote: > On 2013年04月27日 01:34:27, Tad Glines wrote: > > I cannot recall this CL being discussed at the mailing list _prior_ to entering > the review process[1]. Also, it seems to ignore [2]. > > I suggest to reject the CL. > > [1]: http://golang.org/doc/contribute.html#Design > [2]: http://golang.org/doc/go1compat.html I wrote the code for myself. I figured others (including the golang project) might be interested in it. I figured an easy way to make the code available to others is to create a review. Regarding [2]: The code does not break any of the existing tests and, to the best of my knowledge, does not violate the compatibility requirements.
On 2013年04月27日 13:09:12, julienschmidt wrote: > I have not reviewed your complete code yet, these are just a few first thoughts. > > Pool should be a sub-package like Driver if possible. That was my first thought as well. The problem is that I didn't see a way to do it that didn't either create a circular dependency or result in parts of the drive spec in two places. > Is the concept of chainable Drivers really necessary? I find it pretty > confusing. With the ability to chain drivers, it is possible for a developer to add additional functionality. For example: SQL->Pool->Logging->DB Driver or SQL->Pool->Row Cache->DB Driver > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go > File src/pkg/database/sql/pool.go (right): > > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... > src/pkg/database/sql/pool.go:80: func (c *PoolConfig) fixValues() { > return an error / panic instead of silently fixing the config? This is by design (see the comments on the fields in PoolConfig). Returning an error instead is certainly an option, but in this case I though normalizing the values was better. > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... > src/pkg/database/sql/pool.go:149: } else if c, ok := config.(string); ok { > I think this part should be removed. One way to configure the pool is enough. I was aiming for flexibility but, given that it currently add a dependency on encoding/json, I'm willing to remove the option to configure using strings. > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/sql.go > File src/pkg/database/sql/sql.go (right): > > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/sql.go#... > src/pkg/database/sql/sql.go:213: func Open(driverName, dataSourceName > interface{}) (*DB, error) { > I'm not sure if this breaks the API contract but I don't think this is a good > idea anyways. It might be better to have a separate OpenChain(...) function. > This avoids much confusion and possible errors. I thought about this as well. Initially I had left the Open() API unchanged and required that a chain be configured by naming the drivers as, for example, "[driver1, driver1]", and having to escape the " in the JSON encoded config. And it just seemed overly convoluted. On balance, I think adding OpenChain() will be cleaner.
Just some comments about error handling. Another thing I noted: You RLock/RUnlock the database in many operations. Can't the lock be held during the lifetime of a connection? The DB lock might be very hot otherwise, since the DB is a program wide singleton per database backend, if I understand correctly. https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/driver/... File src/pkg/database/sql/driver/driver.go (right): https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/driver/... src/pkg/database/sql/driver/driver.go:76: var ErrDBLimitExceeded = errors.New("driver: db limit exceeded") Might be useful to have a type here to enrich the error with the name and the value(?) of the limit we hit, if known. This way the these limits can be raised, if hit often. https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/driver/... src/pkg/database/sql/driver/driver.go:83: var ErrSessionLimitExceeded = errors.New("driver: per session limit exceeded") Same comment as above: Please make it a error type and include the actual limit name and value. Maybe even use the same type? https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go File src/pkg/database/sql/pool.go (right): https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... src/pkg/database/sql/pool.go:264: conns map[*poolConn]bool Where is this pool.conns field used? could not find any reference.
Patch Set 4 addresses some comments (I'll reply to those individually) and fixes some additional bugs I found. I added manyConcurrentQueriesWithTx(t) because concurrency through the Tx follows different code paths. After adding that, I found a couple more issues that needed to be fixed.
On 2013年04月27日 13:09:12, julienschmidt wrote: > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... > src/pkg/database/sql/pool.go:149: } else if c, ok := config.(string); ok { > I think this part should be removed. One way to configure the pool is enough. Done. > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/sql.go > File src/pkg/database/sql/sql.go (right): > > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/sql.go#... > src/pkg/database/sql/sql.go:213: func Open(driverName, dataSourceName > interface{}) (*DB, error) { > I'm not sure if this breaks the API contract but I don't think this is a good > idea anyways. It might be better to have a separate OpenChain(...) function. > This avoids much confusion and possible errors. Done.
On 2013年04月27日 23:50:40, ioe wrote: > Just some comments about error handling. > > Another thing I noted: You RLock/RUnlock the database in many operations. > > Can't the lock be held during the lifetime of a connection? > > The DB lock might be very hot otherwise, since the DB is a program wide > singleton per database backend, if I understand correctly. My assumption is that when a developer calls db.Close() they want the DB closed NOW. So, to make sure that the close happens cleanly I read lock the DB during all calls into the code except during Close which does a write lock instead. This ensures that Close gains exclusive access to the data and that once it returns all subsequent calls to DB will return a "db closed" error. > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/driver/... > File src/pkg/database/sql/driver/driver.go (right): > > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/driver/... > src/pkg/database/sql/driver/driver.go:76: var ErrDBLimitExceeded = > errors.New("driver: db limit exceeded") > Might be useful to have a type here to enrich the error with the name and the > value(?) of the limit we hit, if known. > > This way the these limits can be raised, if hit often. Quite often (PostgreSQL for example) the error returned by the DB doesn't say what the limit was, just that it was exceeded. I could define a driver.ErrorDecoder interface that has a GetErrorCode method that returns codes for DBLimitExceeded and SessionLimitExceeded. Then if the driver returns an error that implements that interface, the database/sql code could use it to get the driver to translate the error to error codes it understands. > https://codereview.appspot.com/8797045/diff/6001/src/pkg/database/sql/pool.go... > src/pkg/database/sql/pool.go:264: conns map[*poolConn]bool > Where is this pool.conns field used? could not find any reference. Thanks for catching that. I deleted it in patch set 4.
Hi Tad, many thanks for this CL. I was already waiting eagerly for the abstraction of the Pool interface, because my experimental Cassandra driver doesn't really work with the existing pool capabilities of "database/sql" (in fact, they are very annoying in the case of a Cassandandra backend with multiple hosts). Unfortunately, the Go team is currently prepearing the 1.1 release and no new features are going to be accepted. Therefore I suggest waiting a bit. Add Brad Fitzpatrick to the list of reviewers after the release. PS: And don't give up easily. It's always hard to get such massive changes submitted, but it's worth the effort! Regards, Christoph
> many thanks for this CL. I was already waiting eagerly for the abstraction of > the Pool interface, because my experimental Cassandra driver doesn't really work > with the existing pool capabilities of "database/sql" (in fact, they are very > annoying in the case of a Cassandandra backend with multiple hosts). When designing the Pool interface and chaining capability I was thinking specifically of cases where the backend might maintain connections to more than one instance. Cassandra is on case. Another is the case where you have one master and many slaves or where sharding is used. By making the pooling plugable, it should allow developers to start with a single traditional DB and then drop in a shard-aware or master+slaves aware pool implementation. > Unfortunately, the Go team is currently prepearing the 1.1 release and no new > features are going to be accepted. Therefore I suggest waiting a bit. Add Brad > Fitzpatrick to the list of reviewers after the release. I was not expecting this to make it into go1.1. Something this big may take some time and, given the consensus projects like this, it may not get added in its current form. > PS: And don't give up easily. It's always hard to get such massive changes > submitted, but it's worth the effort! Thanks for the encouragement.
Hi Tad, found some timing related nits. TL;DR: I suggest to use use time.Duration for all delays, timeouts, etc. Best Regards Ingo https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.go File src/pkg/database/sql/pool.go (right): https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:31: AcquireTimeout int Please use time.Duration. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:44: RetryDelay int RetryDelay field seems unused. If it is really used, please use time.Duration for this. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:48: IdleTimeout int Please use time.Duration. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:400: maxDelta := time.Millisecond * time.Duration(p.config.IdleTimeout) Using Duration you can elide this variable and whole line. Also all the type conversions in this function. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:571: r.timer = time.AfterFunc(time.Duration(p.config.AcquireTimeout)*time.Millisecond, r.timeout) using p.config.AcquireTimeout as time.Duration would shorten this line to: r.timer = time.AfterFunc(p.config.AcquireTimeout, r.timeout)
All time durations in the config are now of type time.Duration. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.go File src/pkg/database/sql/pool.go (right): https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:31: AcquireTimeout int On 2013年05月05日 21:27:21, ioe wrote: > Please use time.Duration. Done. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:44: RetryDelay int On 2013年05月05日 21:27:21, ioe wrote: > RetryDelay field seems unused. If it is really used, please use time.Duration > for this. Done. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:48: IdleTimeout int On 2013年05月05日 21:27:21, ioe wrote: > Please use time.Duration. Done. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:400: maxDelta := time.Millisecond * time.Duration(p.config.IdleTimeout) On 2013年05月05日 21:27:21, ioe wrote: > Using Duration you can elide this variable and whole line. > Also all the type conversions in this function. Done. https://codereview.appspot.com/8797045/diff/18001/src/pkg/database/sql/pool.g... src/pkg/database/sql/pool.go:571: r.timer = time.AfterFunc(time.Duration(p.config.AcquireTimeout)*time.Millisecond, r.timeout) On 2013年05月05日 21:27:21, ioe wrote: > using p.config.AcquireTimeout as time.Duration would shorten this line to: > r.timer = time.AfterFunc(p.config.AcquireTimeout, r.timeout) Done.
I've uploaded patch set 5. This patch set includes: - Changes to address comments by ioe - Expansion of concurrency tests/benchmarks to include all major code paths. - Modified OpenChain to be variadic. - Added Execer and Querier implementations to pool. This provides a better performance in cases where the user is calling DB.Exec or DB.Query. - Added code to fakeDriver so concurrency expectations are ensured. Specifically, if more than one goroutine attempt to use the same connection (or any of it's associated statements or rows) at the same time, a panic will occur. - Added rows opens/closes tracking in the fakeConn so that an error can be returned if a an attempt is made to close a connection before the associated rows have been closed. - Minor tweeks.
Patch set 6 includes the following: - Re-added TestStmtCloseDeps and fixed the issues found by it - Removed dependency on container/list - Added NumConcurrentOpens to PoolConfig and modified pool logic so that only NumConcurrentOpens concurrent opens can be active. The default is 1. - Normalized the comments that specify what locks are assumed to be held. - Some cleanups and minor tweaks
Patch set 8 include the list implementation files that I forgot to include in patch set 6.
This is too big to go in as-is. I think it's even too big to go in via a dozen small pieces. If you really like this design, you'd be better off putting it on github. If you want to help improve the existing design's performance/bugs, though, I'm very interested, as long as it doesn't involve so many API additions as in this CL.
On 2013年05月22日 01:31:30, bradfitz wrote: > This is too big to go in as-is. Julien expressed the same opinion. > I think it's even too big to go in via a dozen small pieces. > > If you really like this design, you'd be better off putting it on github. > > If you want to help improve the existing design's performance/bugs, though, I'm > very interested, as long as it doesn't involve so many API additions as in this > CL. Fundamentally, this CL was about adding support for connection limits. I started out by trying to add it in a driver implementation but this turned out to be problematic for four reasons: 1. I didn't want to have to fork an existing driver implementation just to add connection limits. 2. Adding an intermediate "driver", while possible, is hack'ish at best without exposing driver/sql internals. 3. If the driver imposes connection limits, then you either have to set MaxIdleConnections to 0 in order to avoid a potential "deadlock" (e.g. one go routine is waiting on driver.Open, but all the existing connections are in use, or in the idle pool), or you have to implement code in driver/sql to deal with an ErrConnectionLimitsExceeded fron the driver.Open 4. If MaxIdleConnections is 0 in order to allow connections limits to work, then, in order to again have a prepared statement cache, one would have to implements it again in the driver (intermediate or root) and end up with a longer more complicated code path than is strictly necessary. Are you willing to consider a CL that add connections limit logic in database/sql? Also, are you willing to consider a CL that adds the ability to add one or more intermediate "filter" drivers? This capability could be used for collecting statistics or implementing results caching among other things.
Sorry for the slow response. I'm totally cool with adding connection limit logic to database/sql. Filter drivers get more complicated because they'd shadow optional interface methods on drivers. I'm off on vacation for a week and will likely have no connectivity. On Tue, May 21, 2013 at 7:54 PM, <Tad.Glines@gmail.com> wrote: > On 2013年05月22日 01:31:30, bradfitz wrote: > >> This is too big to go in as-is. >> > > Julien expressed the same opinion. > > > I think it's even too big to go in via a dozen small pieces. >> > > If you really like this design, you'd be better off putting it on >> > github. > > If you want to help improve the existing design's performance/bugs, >> > though, I'm > >> very interested, as long as it doesn't involve so many API additions >> > as in this > >> CL. >> > > Fundamentally, this CL was about adding support for connection limits. I > started out by trying to add it in a driver implementation but this > turned out to be problematic for four reasons: > 1. I didn't want to have to fork an existing driver implementation just > to add connection limits. > 2. Adding an intermediate "driver", while possible, is hack'ish at best > without exposing driver/sql internals. > 3. If the driver imposes connection limits, then you either have to set > MaxIdleConnections to 0 in order to avoid a potential "deadlock" (e.g. > one go routine is waiting on driver.Open, but all the existing > connections are in use, or in the idle pool), or you have to implement > code in driver/sql to deal with an ErrConnectionLimitsExceeded fron the > driver.Open > 4. If MaxIdleConnections is 0 in order to allow connections limits to > work, then, in order to again have a prepared statement cache, one would > have to implements it again in the driver (intermediate or root) and end > up with a longer more complicated code path than is strictly necessary. > > Are you willing to consider a CL that add connections limit logic in > database/sql? > Also, are you willing to consider a CL that adds the ability to add one > or more intermediate "filter" drivers? This capability could be used for > collecting statistics or implementing results caching among other > things. > > > https://codereview.appspot.**com/8797045/<https://codereview.appspot.com/8797... >
On 2013年05月25日 16:57:29, bradfitz wrote: > Sorry for the slow response. I assumed you are busy. > I'm totally cool with adding connection limit logic to database/sql. Ok. I'll start working on a CL for that. > Filter drivers get more complicated because they'd shadow optional > interface methods on drivers. All optional interfaces are specified as being able to return ErrSkip. A filter would be expected to implement all optional interfaces but return ErrSkip if the underlying driver doesn't implement the interface.
On Sat, May 25, 2013 at 10:06 AM, <Tad.Glines@gmail.com> wrote: > On 2013年05月25日 16:57:29, bradfitz wrote: > >> Sorry for the slow response. >> > > I assumed you are busy. > > > I'm totally cool with adding connection limit logic to database/sql. >> > > Ok. I'll start working on a CL for that. > > > Filter drivers get more complicated because they'd shadow optional >> interface methods on drivers. >> > > All optional interfaces are specified as being able to return ErrSkip. A > filter would be expected to implement all optional interfaces but return > ErrSkip if the underlying driver doesn't implement the interface. > But then we can't add new optional methods as easily in the future. We have to trust the filter to always have all methods, or just require that filters implement the world. Alternative, we could require the filter have a method to get at the underlying driver, so the database/sql package could test the real driver interface value directly. I haven't thought about it much. I'd like to avoid it until actually necessary.
On 2013年05月25日 16:57:29, bradfitz wrote: > I'm totally cool with adding connection limit logic to database/sql. The CL for this is here: https://codereview.appspot.com/9789044/