Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(608)

Issue 8836045: code review 8836045: database/sql: fix driver Conn refcounting with prepared... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by bradfitz
Modified:
10 years, 11 months ago
Reviewers:
brainman, kardia, mtnngw
CC:
golang-dev, snaury, gwenn, julienschmidt, r
Visibility:
Public.

Description

database/sql: fix driver Conn refcounting with prepared statements The refcounting of driver Conns was completedly busted and would leak (be held open forever) with any reasonable load. This was a significant regression from Go 1.0. The core of this patch is removing one line: s.db.addDep(dc, s) A database conn (dc) is a resource that be re-created any time (but cached for speed) should not be held open forever with a dependency refcount just because the Stmt (s) is alive (which typically last for long periods of time, like forever). The meat of the patch is new tests. In fixing the real issue, a lot of tests then failed due to the fakedb_test.go's paranoia about closing a fakeConn while it has open fakeStmts on it. I could've ignored that, but that's been a problem in the past for other bugs. Instead, I now track per-Conn open statements and close them when the the conn closes. The proper way to do this would've been making *driverStmt a finalCloser and using the dep mechanism, but it was much more invasive. Added a TODO instead. I'd like to give a way for drivers to opt-out of caring about driver.Stmt closes before a driver.Conn close, but that's a TODO for the future, and that TODO is added in this CL. I know this is very late for Go 1.1, but database/sql is currently nearly useless without this. I'd like to believe all these database/sql bugs in the past release cycle are the result of increased usage, number of drivers, and good feedback from increasingly-capable Go developers, and not the result of me sucking. It's also hard with all the real drivers being out-of-tree, so I'm having to add more and more hooks to fakedb_test.go to simulate things which real drivers end up doing. Fixes Issue 5323

Patch Set 1 #

Patch Set 2 : diff -r 37bf155bc780 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 37bf155bc780 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 37bf155bc780 https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 5 : diff -r 0c16e97c7587 https://go.googlecode.com/hg/ #

Total comments: 5

Patch Set 6 : diff -r 0c16e97c7587 https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 7 : diff -r 0c16e97c7587 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r 1d079908dd84 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+296 lines, -36 lines) Patch
M src/pkg/database/sql/fakedb_test.go View 1 2 3 4 5 6 5 chunks +20 lines, -4 lines 0 comments Download
M src/pkg/database/sql/sql.go View 1 2 3 4 5 6 7 15 chunks +98 lines, -27 lines 0 comments Download
M src/pkg/database/sql/sql_test.go View 1 2 3 4 5 6 10 chunks +178 lines, -5 lines 0 comments Download

Messages

Total messages: 21
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 11 months ago (2013-04-24 19:45:40 UTC) #1
snaury
On 2013/04/24 19:45:40, bradfitz wrote: > I'd like you to review this change to > ...
10 years, 11 months ago (2013-04-24 20:34:42 UTC) #2
bradfitz
Hello golang-dev@googlegroups.com, snaury@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 11 months ago (2013-04-24 23:17:23 UTC) #3
bradfitz
On Wed, Apr 24, 2013 at 1:34 PM, <snaury@gmail.com> wrote: > On 2013/04/24 19:45:40, bradfitz ...
10 years, 11 months ago (2013-04-24 23:17:53 UTC) #4
bradfitz
[+julienschmidt, golang-nuts] Could database/sql users please test with this patch and report back? I'd like ...
10 years, 11 months ago (2013-04-25 02:23:14 UTC) #5
snaury
Needs some changes. For example, under load with concurrency 50 (I added a goroutine in ...
10 years, 11 months ago (2013-04-25 05:56:36 UTC) #6
snaury
On 2013/04/25 05:56:36, snaury wrote: > Before changes: Stmt: css = 14895 elements (14895 cap) ...
10 years, 11 months ago (2013-04-25 05:58:36 UTC) #7
bradfitz
Wow, embarrassing. As penance, I've added more tests and fixed a couple other bugs (openStmt ...
10 years, 11 months ago (2013-04-25 07:17:06 UTC) #8
bradfitz
Hello golang-dev@googlegroups.com, snaury@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 11 months ago (2013-04-25 07:17:14 UTC) #9
gwenn
Seems good to me (with sqlite driver). On Thursday, April 25, 2013 4:23:14 AM UTC+2, ...
10 years, 11 months ago (2013-04-25 17:55:29 UTC) #10
snaury
LGTM, but see comments https://codereview.appspot.com/8836045/diff/20001/src/pkg/database/sql/sql.go File src/pkg/database/sql/sql.go (right): https://codereview.appspot.com/8836045/diff/20001/src/pkg/database/sql/sql.go#newcode695 src/pkg/database/sql/sql.go:695: si, err := dc.prepareLocked(query) I ...
10 years, 11 months ago (2013-04-25 19:01:34 UTC) #11
bradfitz
Yeah, I was debating consistency vs. performance (not tracking them) on those ephemeral prepareLocked/closeStatement calls. ...
10 years, 11 months ago (2013-04-25 19:09:05 UTC) #12
bradfitz
Hello golang-dev@googlegroups.com, snaury@gmail.com, gwenn.kahz@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 11 months ago (2013-04-25 19:19:35 UTC) #13
snaury
On 2013/04/25 19:19:35, bradfitz wrote: > Please take another look. LGTM
10 years, 11 months ago (2013-04-25 19:23:05 UTC) #14
bradfitz
R=r On Thu, Apr 25, 2013 at 12:23 PM, <snaury@gmail.com> wrote: > On 2013/04/25 19:19:35, ...
10 years, 11 months ago (2013-04-25 19:25:56 UTC) #15
julienschmidt
As far as I can tell, this SGTM All my tests pass and the performance ...
10 years, 11 months ago (2013-04-25 19:26:59 UTC) #16
r
LGTM fingers crossed https://codereview.appspot.com/8836045/diff/8001/src/pkg/database/sql/fakedb_test.go File src/pkg/database/sql/fakedb_test.go (right): https://codereview.appspot.com/8836045/diff/8001/src/pkg/database/sql/fakedb_test.go#newcode38 src/pkg/database/sql/fakedb_test.go:38: mu sync.Mutex // guards all 4 ...
10 years, 11 months ago (2013-04-25 20:09:16 UTC) #17
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=45c12efb4635 *** database/sql: fix driver Conn refcounting with prepared statements The refcounting ...
10 years, 11 months ago (2013-04-25 21:46:10 UTC) #18
brainman
Cannot see any problems with http://code.google.com/p/odbc/ Alex
10 years, 11 months ago (2013-04-26 02:07:29 UTC) #19
mtnngw_gmail.com
With Tip and with just before the change: $ go version go version devel +5e23075b0970 ...
10 years, 11 months ago (2013-04-26 21:49:22 UTC) #20
kardia
10 years, 11 months ago (2013-04-26 23:43:33 UTC) #21
Hi Matt N,

Is it possible to show your code? I'd be interested to see it. 

Thanks,
-Daniel


On Friday, April 26, 2013 2:49:21 PM UTC-7, Matt N wrote:
>
> With Tip and with just before the change:
>
> $ go version
> go version devel +5e23075b0970 Fri Apr 26 11:42:58 2013 -0700 darwin/amd64
>
> //*sql.DB connection pool size = 20
> BenchmarkCreate        1000        609021 ns/op
> BenchmarkRead        2000        379022 ns/op
> BenchmarkDelete        5000        296088 ns/op
>
> // no connection pool
> BenchmarkCreate         100       6199264 ns/op
> BenchmarkRead         200       4424808 ns/op
> BenchmarkDelete         500       3396262 ns/op
>
>
> $ go version
> go version devel +1d079908dd84 Thu Apr 25 18:47:12 2013 +0200 darwin/amd64
>
> //*sql.DB connection pool size = 20
> BenchmarkCreate        1000        609492 ns/op
> BenchmarkRead        2000        386040 ns/op
> BenchmarkDelete        5000        299545 ns/op
>
>
> This is a rMBP, SSD, 2.6 Ghz Intel Core i7, 16 GB memory.
> PostgreSQL 9.2.4
> github.com/lib/pq
>
> With a home brewed connection pool, I get basically the same performance.  
> If I don't recycle my sql.DB pointers, I get 10x worse.
>
> With *sql.DB connection pool size set to 50, I get "pq: sorry, too many 
> clients already." in both versions.
>
> For me, it seems that w/ respect to postgres, (and the github/lib/pq 
> driver) everything is the same...
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b