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.