code review 131650043: database/sql: Close per-tx prepared statements when the...

database/sql: Close per-tx prepared statements when the associated tx ends

[johto, 2014-08-29 03:02:08]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[johto, 2014-08-29 03:05:41]

Hi,

On 8/29/14, 5:02 AM, I wrote:
> I'd like you to review this change to
> https://code.google.com/p/go
>
> Description:
> database/sql: Close per-tx prepared statements when the associated tx
> ends

This is the change discussed in issue 114400043.  This should also fix 
common problems with deferring the close of a tx-scoped prepared 
statement and the leaking of prepared statements when following the 
usage pattern described in the *Tx.Stmt documentation.


.marko

[bradfitz, 2014-09-02 16:22:49]

https://codereview.appspot.com/131650043/diff/10002/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/131650043/diff/10002/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1189: tx.stmts = append(tx.stmts, txstmt)
previously it was safe to concurrently call tx.Stmt, at least in theory. We
never said anything either way, but the rest of the database/sql APIs are safe
for use by concurrent goroutines, so it stands to reason people would assume
this is too.  I think we might need a mutex guarding tx.stmts to not change the
API rules on people. Thoughts?

[johto, 2014-09-02 19:33:23]

On 2014-09-02 6:22 PM, bradfitz@golang.org wrote:
>
https://codereview.appspot.com/131650043/diff/10002/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1189: tx.stmts = append(tx.stmts, txstmt)
> previously it was safe to concurrently call tx.Stmt, at least in theory.
> We never said anything either way, but the rest of the database/sql APIs
> are safe for use by concurrent goroutines, so it stands to reason people
> would assume this is too.  I think we might need a mutex guarding
> tx.stmts to not change the API rules on people. Thoughts?

*Tx not specifying whether it's safe to be used concurrently has always 
meant to me that it's *not* safe.  But I don't really have a problem 
with guarding it with a mutex in this case.  I'll update the CL.


.marko

[johto, 2014-09-02 22:03:36]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-09-02 22:17:00]

https://codereview.appspot.com/131650043/diff/70001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/131650043/diff/70001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1047: stmtMutex sync.Mutex // protects stmts
stmtsMu sync.Mutex // guards stmts

alternatively, you can do:

  stmts struct {
      sync.Mutex
      v []*Stmt
  }

And then you can write:

   stmts.Lock()
   stmts.v = append(stmts.v, x)
   stmts.Unlock()

https://codereview.appspot.com/131650043/diff/70001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1075: for _, stmt := range tx.stmts {
missing tx.stmtsMu.Lock() / defer tx.stmtsMu.Unlock()

https://codereview.appspot.com/131650043/diff/70001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1076: _ = stmt.Close()
drop the "_ ="

[johto, 2014-09-02 23:16:17]

On 2014-09-03 00:16, bradfitz@golang.org wrote:
> alternatively, you can do:
>
>     stmts struct {
>         sync.Mutex
>         v []*Stmt
>     }

I went with this approach instead, it seemed cleaner.

>
https://codereview.appspot.com/131650043/diff/70001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1076: _ = stmt.Close()
> drop the "_ ="

Done.

>
https://codereview.appspot.com/131650043/diff/70001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1075: for _, stmt := range tx.stmts {
> missing tx.stmtsMu.Lock() / defer tx.stmtsMu.Unlock()

Yeah, that was an oversight.  Fixed.

Any additional comments?


.marko

[mattn, 2014-09-03 06:29:13]

Is this related to https://code.google.com/p/go/issues/detail?id=6734 ?

[johto, 2014-09-03 07:32:21]

On 2014-09-03 08:29, mattn.jp@gmail.com wrote:
> Is this related to https://code.google.com/p/go/issues/detail?id=6734 ?

No, this doesn't fix that problem at all (though that issue is probably 
worth fixing as well).


.marko

[bradfitz, 2014-09-16 19:32:29]

https://codereview.appspot.com/131650043/diff/90001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/131650043/diff/90001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1029: type stmts struct {
don't make this a top-level named type.  just write it below.  It's easier to
read that way.

https://codereview.appspot.com/131650043/diff/90001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:1188: txstmt := &Stmt{
just "txs" is probably fine. that variable is a mouthful.

[johto, 2014-09-21 23:54:34]

Hi Brad,

On 9/16/14, 9:32 PM, bradfitz@golang.org wrote:
>
https://codereview.appspot.com/131650043/diff/90001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1029: type stmts struct {
> don't make this a top-level named type.  just write it below.  It's
> easier to read that way.

I've updated the CL; did you mean like that?

>
https://codereview.appspot.com/131650043/diff/90001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:1188: txstmt := &Stmt{
> just "txs" is probably fine. that variable is a mouthful.

All righty.  Fixed.


.marko

[bradfitz, 2014-09-22 13:18:55]

LGTM


On Sun, Sep 21, 2014 at 7:54 PM, Marko Tiikkaja <marko@joh.to> wrote:

> Hi Brad,
>
> On 9/16/14, 9:32 PM, bradfitz@golang.org wrote:
>
>> https://codereview.appspot.com/131650043/diff/90001/src/
>> pkg/database/sql/sql.go#newcode1029
>> src/pkg/database/sql/sql.go:1029: type stmts struct {
>> don't make this a top-level named type.  just write it below.  It's
>> easier to read that way.
>>
>
> I've updated the CL; did you mean like that?
>
>  https://codereview.appspot.com/131650043/diff/90001/src/
>> pkg/database/sql/sql.go#newcode1188
>> src/pkg/database/sql/sql.go:1188: txstmt := &Stmt{
>> just "txs" is probably fine. that variable is a mouthful.
>>
>
> All righty.  Fixed.
>
>
> .marko
>

[bradfitz, 2014-09-22 13:19:23]

*** Submitted as https://code.google.com/p/go/source/detail?r=50ce4ec65c4a ***

database/sql: Close per-tx prepared statements when the associated tx ends

LGTM=bradfitz
R=golang-codereviews, bradfitz, mattn.jp
CC=golang-codereviews
https://codereview.appspot.com/131650043

Committer: Brad Fitzpatrick <bradfitz@golang.org>