Hi, On 8/29/14, 5:02 AM, I wrote: > I'd like you to review this change ...
10 years, 3 months ago
(2014-08-29 03:05:41 UTC)
#2
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
10 years, 3 months ago
(2014-09-02 19:33:23 UTC)
#4
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
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.go#newcode1029 src/pkg/database/sql/sql.go:1029: type stmts struct { don't make this a top-level ...
10 years, 2 months ago
(2014-09-16 19:32:29 UTC)
#10
LGTM On Sun, Sep 21, 2014 at 7:54 PM, Marko Tiikkaja <marko@joh.to> wrote: > Hi ...
10 years, 2 months ago
(2014-09-22 13:18:55 UTC)
#12
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
>
*** Submitted as https://code.google.com/p/go/source/detail?r=50ce4ec65c4a *** database/sql: Close per-tx prepared statements when the associated tx ends ...
10 years, 2 months ago
(2014-09-22 13:19:23 UTC)
#13
Issue 131650043: code review 131650043: database/sql: Close per-tx prepared statements when the...
Created 10 years, 3 months ago by johto
Modified 10 years, 2 months ago
Reviewers:
Base URL:
Comments: 6