Issue 10726044: database/sql: add SetMaxOpenConns (Closed)
|
Created:
11 years, 9 months ago by Tad Glines Modified:
11 years, 7 months ago Reviewers:
bradfitz CC:
bradfitz, golang-dev Visibility:
Public. |
Descriptiondatabase/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 separate benchmarks for each path for Exec and Query.
Replaced slice based idle pool with list based idle pool.
Patch Set 1 #Patch Set 2 : diff -r fccd815ed3bb https://code.google.com/p/go #
Total comments: 4
Patch Set 3 : diff -r cd6748938734 https://code.google.com/p/go #Patch Set 4 : diff -r cd6748938734 https://code.google.com/p/go #
Total comments: 21
Patch Set 5 : diff -r cd6748938734 https://code.google.com/p/go #Patch Set 6 : diff -r 6782fb307e4e https://code.google.com/p/go #
Total comments: 18
Patch Set 7 : diff -r 6782fb307e4e https://code.google.com/p/go #Patch Set 8 : diff -r 6939fe74ea98 https://code.google.com/p/go #
Total comments: 2
Patch Set 9 : diff -r 6939fe74ea98 https://code.google.com/p/go #
MessagesTotal messages: 24
https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go File src/pkg/database/sql/list.go (right): https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.g... src/pkg/database/sql/list.go:3: type list struct { document what this file and/or this type is. if it's just a list, the language has slices. so explain what you're trying to implement. (It's even possible you implemented it correctly, but you never say what it is) https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.g... src/pkg/database/sql/list.go:17: node_data() *listNodeData no underscores. nodeData.
Sign in to reply to this message.
Ping. What's the status on this? Do you still want to finish it? Go 1.2 draws ever nearer and I'd like to get this in. On Wed, Jul 3, 2013 at 12:30 AM, <bradfitz@golang.org> wrote: > > https://codereview.appspot.**com/10726044/diff/3001/src/** > pkg/database/sql/list.go<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go> > File src/pkg/database/sql/list.go (right): > > https://codereview.appspot.**com/10726044/diff/3001/src/** > pkg/database/sql/list.go#**newcode3<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go#newcode3> > src/pkg/database/sql/list.go:**3: type list struct { > document what this file and/or this type is. > > if it's just a list, the language has slices. so explain what you're > trying to implement. (It's even possible you implemented it correctly, > but you never say what it is) > > https://codereview.appspot.**com/10726044/diff/3001/src/** > pkg/database/sql/list.go#**newcode17<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go#newcode17> > src/pkg/database/sql/list.go:**17: node_data() *listNodeData > no underscores. nodeData. > > https://codereview.appspot.**com/10726044/<https://codereview.appspot.com/107... >
Sign in to reply to this message.
https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go File src/pkg/database/sql/list.go (right): https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.g... src/pkg/database/sql/list.go:3: type list struct { On 2013/07/02 14:30:20, bradfitz wrote: > document what this file and/or this type is. > > if it's just a list, the language has slices. so explain what you're trying to > implement. (It's even possible you implemented it correctly, but you never say > what it is) I've added some comments that, I hope, are sufficient. When implementing the connection request queue I wanted the requests to handled in FIFO order. Doing this with a slice would have made the remove operation O(n). I changed freeConn to a list as well so that the connIfFree operation could also be an O(1) instead of O(n) operation. I could have used containers/list, but by using a composeable listNodeData we save on allocation/garbage collection costs. https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.g... src/pkg/database/sql/list.go:17: node_data() *listNodeData On 2013/07/02 14:30:20, bradfitz wrote: > no underscores. nodeData. Done
Sign in to reply to this message.
I sympathize with your quest to eliminate garbage and improve performance, but can we keep this CL about one thing at a time? How do you feel about getting this in with container/list first, and then doing list.go in a separate CL, with its own benchcmp performance numbers in the CL description? I would rather ignore all that discussion right now and only focus on the database/sql issues. Thanks for continuing to work on this. I want to get it in. On Thu, Jul 18, 2013 at 8:44 AM, <Tad.Glines@gmail.com> wrote: > Reviewers: bradfitz, > > > > https://codereview.appspot.**com/10726044/diff/3001/src/** > pkg/database/sql/list.go<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go> > File src/pkg/database/sql/list.go (right): > > https://codereview.appspot.**com/10726044/diff/3001/src/** > pkg/database/sql/list.go#**newcode3<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go#newcode3> > src/pkg/database/sql/list.go:**3: type list struct { > On 2013/07/02 14:30:20, bradfitz wrote: > >> document what this file and/or this type is. >> > > if it's just a list, the language has slices. so explain what you're >> > trying to > >> implement. (It's even possible you implemented it correctly, but you >> > never say > >> what it is) >> > > I've added some comments that, I hope, are sufficient. > > When implementing the connection request queue I wanted the requests to > handled in FIFO order. Doing this with a slice would have made the > remove operation O(n). I changed freeConn to a list as well so that the > connIfFree operation could also be an O(1) instead of O(n) operation. > > I could have used containers/list, but by using a composeable > listNodeData we save on allocation/garbage collection costs. > > > https://codereview.appspot.**com/10726044/diff/3001/src/** > pkg/database/sql/list.go#**newcode17<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go#newcode17> > src/pkg/database/sql/list.go:**17: node_data() *listNodeData > On 2013/07/02 14:30:20, bradfitz wrote: > >> no underscores. nodeData. >> > > Done > > Description: > database/sql: Add SetMaxOpenConns(), the chan based alternative > > 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. > > Please review this at https://codereview.appspot.**com/10726044/<https://codereview.appspot.com/107... > > Affected files: > M src/pkg/database/sql/fakedb_**test.go > A src/pkg/database/sql/list.go > A src/pkg/database/sql/list_**test.go > M src/pkg/database/sql/sql.go > M src/pkg/database/sql/sql_test.**go > > >
Sign in to reply to this message.
Also, add to this CL's description, on its own line: Update issue 4805 On Mon, Jul 22, 2013 at 1:29 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > I sympathize with your quest to eliminate garbage and improve performance, > but can we keep this CL about one thing at a time? > > How do you feel about getting this in with container/list first, and then > doing list.go in a separate CL, with its own benchcmp performance numbers > in the CL description? I would rather ignore all that discussion right now > and only focus on the database/sql issues. > > Thanks for continuing to work on this. I want to get it in. > > > > > On Thu, Jul 18, 2013 at 8:44 AM, <Tad.Glines@gmail.com> wrote: > >> Reviewers: bradfitz, >> >> >> >> https://codereview.appspot.**com/10726044/diff/3001/src/** >> pkg/database/sql/list.go<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go> >> File src/pkg/database/sql/list.go (right): >> >> https://codereview.appspot.**com/10726044/diff/3001/src/** >> pkg/database/sql/list.go#**newcode3<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go#newcode3> >> src/pkg/database/sql/list.go:**3: type list struct { >> On 2013/07/02 14:30:20, bradfitz wrote: >> >>> document what this file and/or this type is. >>> >> >> if it's just a list, the language has slices. so explain what you're >>> >> trying to >> >>> implement. (It's even possible you implemented it correctly, but you >>> >> never say >> >>> what it is) >>> >> >> I've added some comments that, I hope, are sufficient. >> >> When implementing the connection request queue I wanted the requests to >> handled in FIFO order. Doing this with a slice would have made the >> remove operation O(n). I changed freeConn to a list as well so that the >> connIfFree operation could also be an O(1) instead of O(n) operation. >> >> I could have used containers/list, but by using a composeable >> listNodeData we save on allocation/garbage collection costs. >> >> >> https://codereview.appspot.**com/10726044/diff/3001/src/** >> pkg/database/sql/list.go#**newcode17<https://codereview.appspot.com/10726044/diff/3001/src/pkg/database/sql/list.go#newcode17> >> src/pkg/database/sql/list.go:**17: node_data() *listNodeData >> On 2013/07/02 14:30:20, bradfitz wrote: >> >>> no underscores. nodeData. >>> >> >> Done >> >> Description: >> database/sql: Add SetMaxOpenConns(), the chan based alternative >> >> 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. >> >> Please review this at https://codereview.appspot.**com/10726044/<https://codereview.appspot.com/107... >> >> Affected files: >> M src/pkg/database/sql/fakedb_**test.go >> A src/pkg/database/sql/list.go >> A src/pkg/database/sql/list_**test.go >> M src/pkg/database/sql/sql.go >> M src/pkg/database/sql/sql_test.**go >> >> >> >
Sign in to reply to this message.
On 2013/07/22 20:31:30, bradfitz wrote: > Also, add to this CL's description, on its own line: > > Update issue 4805 Done. > On Mon, Jul 22, 2013 at 1:29 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > > > I sympathize with your quest to eliminate garbage and improve performance, > > but can we keep this CL about one thing at a time? > > > > How do you feel about getting this in with container/list first, and then > > doing list.go in a separate CL, with its own benchcmp performance numbers > > in the CL description? I would rather ignore all that discussion right now > > and only focus on the database/sql issues. > > > > Thanks for continuing to work on this. I want to get it in. Done.
Sign in to reply to this message.
https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (right): https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:197: freeConn *list.List *list.List // of type *foo https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:198: connRequests *list.List *list.List // of type *foo https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:201: openerCh chan struct{} add a comment above this saying what it's for. who sends and who receives? is it ever closed? https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:226: listElem *list.Element of what? What is its Value interface{} type? https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:414: openerCh: make(chan struct{}, 1000), // This constant needs to be re-considered I'd pull the constant out into a named private constant, with a big comment before it. Why does it need to be re-considered? Can it deadlock? etc. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:554: for { why not just: for _ = range db.openerCh { db.openNewConnection() } https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:567: var dc *driverConn = nil drop = nil https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:593: ch chan interface{} // takes either a *driverConn or an error make this be a send-only channel: ch chan<- interface{} // takes either a *driverConn or an error makes the intention of the type more clear. could also just be: type connRequest chan<- interface{} but struct is fine too. you wanted fewer allocations, though. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:608: req.ch = make(chan interface{}, 1) add a little comment to explain why this is buffered. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:612: ret, ok := <-req.ch how does this work if the putConnDBLocked is also holding db.mu.Lock while we're holding it here? seems like this starves the goroutine trying to give us the data we asked for? https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:614: return nil, errors.New("sql: database is closed") isn't there an errDBClosed variabled? there should be, if not, and use it everywhere.
Sign in to reply to this message.
https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (right): https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:197: freeConn *list.List On 2013/07/22 21:48:33, bradfitz wrote: > *list.List // of type *foo Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:198: connRequests *list.List On 2013/07/22 21:48:33, bradfitz wrote: > *list.List // of type *foo Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:201: openerCh chan struct{} On 2013/07/22 21:48:33, bradfitz wrote: > add a comment above this saying what it's for. who sends and who receives? is it > ever closed? Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:226: listElem *list.Element On 2013/07/22 21:48:33, bradfitz wrote: > of what? What is its Value interface{} type? Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:414: openerCh: make(chan struct{}, 1000), // This constant needs to be re-considered On 2013/07/22 21:48:33, bradfitz wrote: > I'd pull the constant out into a named private constant, with a big comment > before it. Why does it need to be re-considered? Can it deadlock? etc. Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:554: for { On 2013/07/22 21:48:33, bradfitz wrote: > why not just: > > for _ = range db.openerCh { > db.openNewConnection() > } Right. I keep forgetting that range ends when the chan is closed. Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:567: var dc *driverConn = nil On 2013/07/22 21:48:33, bradfitz wrote: > drop = nil Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:608: req.ch = make(chan interface{}, 1) On 2013/07/22 21:48:33, bradfitz wrote: > add a little comment to explain why this is buffered. Done. https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:612: ret, ok := <-req.ch On 2013/07/22 21:48:33, bradfitz wrote: > how does this work if the putConnDBLocked is also holding db.mu.Lock while we're > holding it here? seems like this starves the goroutine trying to give us the > data we asked for? I'm not sure what you are asking here. The db.mu lock is unlocked before reading from the chan (see previous line). https://codereview.appspot.com/10726044/diff/17001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:614: return nil, errors.New("sql: database is closed") On 2013/07/22 21:48:33, bradfitz wrote: > isn't there an errDBClosed variabled? there should be, if not, and use it > everywhere. Done.
Sign in to reply to this message.
Looking better. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (right): https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:197: freeConn *list.List // or driverConn // of *driverConn https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:198: connRequests *list.List // connRequest // of connRequest https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:527: // that this instance of *DB can maintain. s/this instance of *DB/db/ Or just: // SetMaxOpenConns sets the maximum number of open connections to the database. // // If ... https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:531: // MaxOpenConns limit end in period https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:575: var dc *driverConn can you move this down to when you actually need it? https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:585: if err == nil { maybe: db.pendingOpens-- if err != nil { db.putConnDBLocked(nil, err) return } dc := &driverConn{ ... } db.addDepLocked(dc, dc) db.numOpen++ db.putConnDBLocked(dc, nil) ? https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:598: // When there are no idle connections available, conn() will create DB.conn (drop the parens) https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:600: type connRequest chan interface{} // takes either a *driverConn or an error why not chan<- interface{} ? See http://play.golang.org/p/CplZu95RIQ https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:620: db.mu.Unlock() ah, I missed this line before. gotcha. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:762: // Satisfy a connRequest or put the driverConn in the idle pool and return true Write this as a complete sentence (with subject "putConnDBLocked" first) and mention what dc and err mean and when/whether they can be nil.
Sign in to reply to this message.
https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (right): https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:197: freeConn *list.List // or driverConn On 2013/07/23 00:05:19, bradfitz wrote: > // of *driverConn Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:198: connRequests *list.List // connRequest On 2013/07/23 00:05:19, bradfitz wrote: > // of connRequest Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:527: // that this instance of *DB can maintain. On 2013/07/23 00:05:19, bradfitz wrote: > s/this instance of *DB/db/ > > Or just: > > // SetMaxOpenConns sets the maximum number of open connections to the database. > // > // If ... > > Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:531: // MaxOpenConns limit On 2013/07/23 00:05:19, bradfitz wrote: > end in period Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:585: if err == nil { On 2013/07/23 00:05:19, bradfitz wrote: > maybe: > > db.pendingOpens-- > if err != nil { > db.putConnDBLocked(nil, err) > return > } > dc := &driverConn{ > ... > } > db.addDepLocked(dc, dc) > db.numOpen++ > db.putConnDBLocked(dc, nil) > > > ? Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:598: // When there are no idle connections available, conn() will create On 2013/07/23 00:05:19, bradfitz wrote: > DB.conn (drop the parens) Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:600: type connRequest chan interface{} // takes either a *driverConn or an error On 2013/07/23 00:05:19, bradfitz wrote: > why not chan<- interface{} ? > > See http://play.golang.org/p/CplZu95RIQ Done. https://codereview.appspot.com/10726044/diff/31001/src/pkg/database/sql/sql.g... src/pkg/database/sql/sql.go:762: // Satisfy a connRequest or put the driverConn in the idle pool and return true On 2013/07/23 00:05:19, bradfitz wrote: > Write this as a complete sentence (with subject "putConnDBLocked" first) and > mention what dc and err mean and when/whether they can be nil. Done.
Sign in to reply to this message.
This CL continues to scare me. I worry that it will be a maintenance nightmare.
Sign in to reply to this message.
On 2013/08/02 17:24:27, bradfitz wrote: > This CL continues to scare me. > > I worry that it will be a maintenance nightmare. What, specifically, worries you about this CL? Are there parts of the code that seem difficult to comprehend? Are you concerned about some unexpected code interaction or unintended behavioral consequence? I can add more comments if needed, though I'm loth to comment that which seems fairly obvious to me.
Sign in to reply to this message.
It's just a lot of new moving pieces and state, and database/sql wasn't already the most simple package. I was kinda hoping that we'd be able to make the codebase better and easier to understand in the process of fixing this, if maybe we were able to abstract some connection policy out somehow. I'm not worried about lines of code... I'm worried about mental overhead of maintaining this in the future and understanding how all the pieces interact. I still want to get this in before Go 1.2, but I'll need to get more comfortable with it. I think this is the only major CL outstanding that I'm currently worrying about and want to get in. On Fri, Aug 2, 2013 at 12:33 PM, <Tad.Glines@gmail.com> wrote: > On 2013/08/02 17:24:27, bradfitz wrote: > >> This CL continues to scare me. >> > > I worry that it will be a maintenance nightmare. >> > > What, specifically, worries you about this CL? > > Are there parts of the code that seem difficult to comprehend? > Are you concerned about some unexpected code interaction or unintended > behavioral consequence? > I can add more comments if needed, though I'm loth to comment that which > seems fairly obvious to me. > > > https://codereview.appspot.**com/10726044/<https://codereview.appspot.com/107... >
Sign in to reply to this message.
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.
Sign in to reply to this message.
*** 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 <bradfitz@golang.org>
Sign in to reply to this message.
But it breaks the build, so reverting. On Thu, Aug 29, 2013 at 5:20 PM, <bradfitz@golang.org> wrote: > *** Submitted as > https://code.google.com/p/go/**source/detail?r=c9bea548fb6f<https://code.goog... > > 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<https://codereview.appspot.com/1072... > > Committer: Brad Fitzpatrick <bradfitz@golang.org> > > > https://codereview.appspot.**com/10726044/<https://codereview.appspot.com/107... >
Sign in to reply to this message.
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 <bradfitz@golang.org>wrote: > But it breaks the build, so reverting. > > > On Thu, Aug 29, 2013 at 5:20 PM, <bradfitz@golang.org> wrote: > >> *** Submitted as >> https://code.google.com/p/go/**source/detail?r=c9bea548fb6f<https://code.goog... >> >> 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<https://codereview.appspot.com/1072... >> >> Committer: Brad Fitzpatrick <bradfitz@golang.org> >> >> >> https://codereview.appspot.**com/10726044/<https://codereview.appspot.com/107... >> > >
Sign in to reply to this message.
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 <bradfitz@golang.org>wrote: > > > But it breaks the build, so reverting. > > > > > > On Thu, Aug 29, 2013 at 5:20 PM, <mailto:bradfitz@golang.org> wrote: > > > >> *** Submitted as > >> > https://code.google.com/p/go/**source/detail?r=c9bea548fb6f%3Chttps://code.go... > >> > >> 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/10...> > >> > >> Committer: Brad Fitzpatrick <mailto:bradfitz@golang.org> > >> > >> > >> > https://codereview.appspot.**com/10726044/%3Chttps://codereview.appspot.com/1...> > >> > > > > 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.glines@gmail.com".
Sign in to reply to this message.
https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_t... File src/pkg/database/sql/sql_test.go (right): https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_t... 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=c9bea548fb6ff253455f52ba4e844094... Lower case "add", and remove "the chan based alternative" from the subject. Thanks!
Sign in to reply to this message.
https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_t... File src/pkg/database/sql/sql_test.go (right): https://codereview.appspot.com/10726044/diff/51001/src/pkg/database/sql/sql_t... 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
Sign in to reply to this message.
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.glines@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.
Sign in to reply to this message.
The codereview tool handles the Author stuff. Btw, drop the parens from your subject. See https://code.google.com/p/go/source/detail?r=c9bea548fb6ff253455f52ba4e844094.... no () at the end. On Thu, Aug 29, 2013 at 8:43 PM, <Tad.Glines@gmail.com> wrote: > 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.glines@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/<https://codereview.appspot.com/107... >
Sign in to reply to this message.
On 2013/08/30 03:44:33, bradfitz wrote: > The codereview tool handles the Author stuff. > > Btw, drop the parens from your subject. See > https://code.google.com/p/go/source/detail?r=c9bea548fb6ff253455f52ba4e844094.... > no () at the end. Ok, fixed this time. > On Thu, Aug 29, 2013 at 8:43 PM, <mailto:Tad.Glines@gmail.com> wrote: > > > 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.glines@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/%3Chttps://codereview.appspot.com/1...> > >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=bf9de8d2e308 *** 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 separate 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 <bradfitz@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
