code review 89510043: database/sql: Prevent transaction commit from closing t...

database/sql: Prevent transaction commit from closing transaction object on commit error. Fixes issue 7777.

[Thetawaves, 2014-04-19 02:54:24]

Hello golang-codereviews@googlegroups.com (cc: thetawaves@gmail.com), I'd like you to review this change to https://code.google.com/p/go

[bradfitz, 2014-04-19 15:40:52]

You'll need a test too. On Apr 19, 2014 8:29 AM, <Thetawaves@gmail.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com (cc: thetawaves@gmail.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > database/sql: Prevent transaction commit from closing transaction object > on commit error. > Fixes issue 7777. > > Please review this at https://codereview.appspot.com/89510043/ > > Affected files (+13, -6 lines): > M src/pkg/database/sql/sql.go > > > Index: src/pkg/database/sql/sql.go > =================================================================== > --- a/src/pkg/database/sql/sql.go > +++ b/src/pkg/database/sql/sql.go > @@ -1064,10 +1064,13 @@ > if tx.done { > return ErrTxDone > } > - defer tx.close() > tx.dc.Lock() > - defer tx.dc.Unlock() > - return tx.txi.Commit() > + err := tx.txi.Commit() > + tx.dc.Unlock() > + if err == nil { > + tx.close() > + } > + return err > } > > // Rollback aborts the transaction. > @@ -1075,10 +1078,14 @@ > if tx.done { > return ErrTxDone > } > - defer tx.close() > + > tx.dc.Lock() > - defer tx.dc.Unlock() > - return tx.txi.Rollback() > + err := tx.txi.Rollback() > + tx.dc.Unlock() > + if err == nil { > + tx.close() > + } > + return err > } > > // Prepare creates a prepared statement for use within a transaction. > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > 

[johto, 2014-04-19 15:43:18]

On 4/19/14, 5:54 AM, Thetawaves@gmail.com wrote: > Description: > database/sql: Prevent transaction commit from closing transaction object > on commit error. What's the motivation behind this change? I think this would break e.g. lib/pq currently in some cases; it assumes that database/sql only ever calls Commit or Rollback once on a transaction object. .marko 

[bradfitz, 2014-04-19 15:45:02]

Fwiw I haven't looked at the change or bug yet. I was only pointing out that changes to this code without tests don't go in. On Apr 19, 2014 8:43 AM, "Marko Tiikkaja" <marko@joh.to> wrote: > On 4/19/14, 5:54 AM, Thetawaves@gmail.com wrote: > >> Description: >> database/sql: Prevent transaction commit from closing transaction object >> on commit error. >> > > What's the motivation behind this change? I think this would break e.g. > lib/pq currently in some cases; it assumes that database/sql only ever > calls Commit or Rollback once on a transaction object. > > > .marko > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > 

[minux1, 2014-04-19 18:31:40]

On Apr 19, 2014 11:43 AM, "Marko Tiikkaja" <marko@joh.to> wrote: > On 4/19/14, 5:54 AM, Thetawaves@gmail.com wrote: >> Description: >> database/sql: Prevent transaction commit from closing transaction object >> on commit error. > What's the motivation behind this change? I think this would break e.g. lib/pq currently in some cases; it assumes that database/sql only ever calls Commit or Rollback once on a transaction object. yeah, this is a hidden API change. We probably should defer it to Go 1.4. 

[gobot, 2014-12-19 05:18:54]

R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/89510043 is best) in the description in your new CL. Thanks very much.