[go] database/sql: Retry with a fresh connection after maxBadConnRetries

[Marko Tiikkaja (Gerrit)]

Marko Tiikkaja uploaded a change:
https://go-review.googlesource.com/2034

database/sql: Retry with a fresh connection after maxBadConnRetries

Previously if the connection pool was larger than maxBadConnRetries and
there
were a lot of bad connections in the pool (for example if the database
server
was restarted), a query might have failed with an ErrBadConn unnecessarily.
Instead of trying to guess how many times to retry, try maxBadConnRetries
times
and then force a fresh connection to be used for the last attempt. At the
same
time, lower maxBadConnRetries to a smaller value now that it's not that
important to retry so many times from the free connection list.

Change-Id: I6542f151a766a658980fb396fa4880ecf5874e3d
---
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
3 files changed, 138 insertions(+), 39 deletions(-)



diff --git a/src/database/sql/fakedb_test.go
b/src/database/sql/fakedb_test.go
index a993fd4..8cbbb29 100644
--- a/src/database/sql/fakedb_test.go
+++ b/src/database/sql/fakedb_test.go
@@ -89,7 +89,10 @@
stmtsMade int
stmtsClosed int
numPrepare int
- bad bool
+
+ // bad connection tests; see isBad()
+ bad bool
+ stickyBad bool
}

func (c *fakeConn) incrStat(v *int) {
@@ -243,13 +246,15 @@
}

func (c *fakeConn) isBad() bool {
- // if not simulating bad conn, do nothing
- if !c.bad {
+ if c.stickyBad {
+ return true
+ } else if c.bad {
+ // alternate between bad conn and not bad conn
+ c.db.badConn = !c.db.badConn
+ return c.db.badConn
+ } else {
return false
}
- // alternate between bad conn and not bad conn
- c.db.badConn = !c.db.badConn
- return c.db.badConn
}

func (c *fakeConn) Begin() (driver.Tx, error) {
@@ -466,7 +471,7 @@
panic("nil c.db; conn = " + fmt.Sprintf("%#v", c))
}

- if hookPrepareBadConn != nil && hookPrepareBadConn() {
+ if c.stickyBad || (hookPrepareBadConn != nil && hookPrepareBadConn()) {
return nil, driver.ErrBadConn
}

@@ -529,7 +534,7 @@
return nil, errClosed
}

- if hookExecBadConn != nil && hookExecBadConn() {
+ if s.c.stickyBad || (hookExecBadConn != nil && hookExecBadConn()) {
return nil, driver.ErrBadConn
}

@@ -613,7 +618,7 @@
return nil, errClosed
}

- if hookQueryBadConn != nil && hookQueryBadConn() {
+ if s.c.stickyBad || (hookQueryBadConn != nil && hookQueryBadConn()) {
return nil, driver.ErrBadConn
}

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 6e6f246..ab52701 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -459,7 +459,7 @@
// TODO(bradfitz): give drivers an optional hook to implement
// this in a more efficient or more reliable way, if they
// have one.
- dc, err := db.conn()
+ dc, err := db.conn(false)
if err != nil {
return err
}
@@ -629,17 +629,30 @@

var errDBClosed = errors.New("sql: database is closed")

-// conn returns a newly-opened or cached *driverConn
-func (db *DB) conn() (*driverConn, error) {
+// conn returns a newly-opened or cached *driverConn. If bypassFree is
true,
+// any cached connections are not considered and instead a new connection
is
+// always opened.
+func (db *DB) conn(bypassFree bool) (*driverConn, error) {
db.mu.Lock()
if db.closed {
db.mu.Unlock()
return nil, errDBClosed
}

- // If db.maxOpen > 0 and the number of open connections is over the limit
- // and there are no free connection, make a request and wait.
- if db.maxOpen > 0 && db.numOpen >= db.maxOpen && len(db.freeConn) == 0 {
+ // Prefer a free connection, if possible.
+ numFree := len(db.freeConn)
+ if !bypassFree && numFree > 0 {
+ conn := db.freeConn[0]
+ copy(db.freeConn, db.freeConn[1:])
+ db.freeConn = db.freeConn[:numFree-1]
+ conn.inUse = true
+ db.mu.Unlock()
+ return conn, nil
+ }
+
+ // Out of free connections or we were asked not to use one. If we're not
+ // allowed to open any more connections, make a request and wait.
+ if db.maxOpen > 0 && db.numOpen >= db.maxOpen {
// Make the connRequest channel. It's buffered so that the
// connectionOpener doesn't block while waiting for the req to be read.
req := make(chan connRequest, 1)
@@ -648,15 +661,6 @@
db.mu.Unlock()
ret := <-req
return ret.conn, ret.err
- }
-
- if c := len(db.freeConn); c > 0 {
- conn := db.freeConn[0]
- copy(db.freeConn, db.freeConn[1:])
- db.freeConn = db.freeConn[:c-1]
- conn.inUse = true
- db.mu.Unlock()
- return conn, nil
}

db.numOpen++ // optimistically
@@ -820,8 +824,9 @@
}

// maxBadConnRetries is the number of maximum retries if the driver returns
-// driver.ErrBadConn to signal a broken connection.
-const maxBadConnRetries = 10
+// driver.ErrBadConn to signal a broken connection before forcing a new
+// connection to be opened.
+const maxBadConnRetries = 2

// Prepare creates a prepared statement for later queries or executions.
// Multiple queries or executions may be run concurrently from the
@@ -830,22 +835,26 @@
var stmt *Stmt
var err error
for i := 0; i < maxBadConnRetries; i++ {
- stmt, err = db.prepare(query)
+ stmt, err = db.prepare(query, false)
if err != driver.ErrBadConn {
break
}
}
+ // if the connection is still bad, make sure we try to establish a new one
+ if err == driver.ErrBadConn {
+ return db.prepare(query, true)
+ }
return stmt, err
}

-func (db *DB) prepare(query string) (*Stmt, error) {
+func (db *DB) prepare(query string, bypassFree bool) (*Stmt, error) {
// TODO: check if db.driver supports an optional
// driver.Preparer interface and call that instead, if so,
// otherwise we make a prepared statement that's bound
// to a connection, and to execute this prepared statement
// we either need to use this connection (if it's free), else
// get a new connection + re-prepare + execute on that one.
- dc, err := db.conn()
+ dc, err := db.conn(bypassFree)
if err != nil {
return nil, err
}
@@ -872,16 +881,20 @@
var res Result
var err error
for i := 0; i < maxBadConnRetries; i++ {
- res, err = db.exec(query, args)
+ res, err = db.exec(query, args, false)
if err != driver.ErrBadConn {
break
}
}
+ // if the connection is still bad, make sure we try to establish a new one
+ if err == driver.ErrBadConn {
+ return db.exec(query, args, true)
+ }
return res, err
}

-func (db *DB) exec(query string, args []interface{}) (res Result, err
error) {
- dc, err := db.conn()
+func (db *DB) exec(query string, args []interface{}, bypassFree bool) (res
Result, err error) {
+ dc, err := db.conn(bypassFree)
if err != nil {
return nil, err
}
@@ -921,16 +934,20 @@
var rows *Rows
var err error
for i := 0; i < maxBadConnRetries; i++ {
- rows, err = db.query(query, args)
+ rows, err = db.query(query, args, false)
if err != driver.ErrBadConn {
break
}
}
+ // if the connection is still bad, make sure we try to establish a new one
+ if err == driver.ErrBadConn {
+ return db.query(query, args, true)
+ }
return rows, err
}

-func (db *DB) query(query string, args []interface{}) (*Rows, error) {
- ci, err := db.conn()
+func (db *DB) query(query string, args []interface{}, bypassFree bool)
(*Rows, error) {
+ ci, err := db.conn(bypassFree)
if err != nil {
return nil, err
}
@@ -1009,16 +1026,20 @@
var tx *Tx
var err error
for i := 0; i < maxBadConnRetries; i++ {
- tx, err = db.begin()
+ tx, err = db.begin(false)
if err != driver.ErrBadConn {
break
}
}
+ // if the connection is still bad, make sure we try to establish a new one
+ if err == driver.ErrBadConn {
+ return db.begin(true)
+ }
return tx, err
}

-func (db *DB) begin() (tx *Tx, err error) {
- dc, err := db.conn()
+func (db *DB) begin(bypassFree bool) (tx *Tx, err error) {
+ dc, err := db.conn(bypassFree)
if err != nil {
return nil, err
}
@@ -1395,7 +1416,7 @@
// new one.
//
// TODO(bradfitz): or always wait for one? make configurable later?
- dc, err := s.db.conn()
+ dc, err := s.db.conn(false)
if err != nil {
return nil, nil, nil, err
}
diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go
index 34efdf2..f0ef24f 100644
--- a/src/database/sql/sql_test.go
+++ b/src/database/sql/sql_test.go
@@ -1314,6 +1314,79 @@
}
}

+// Test cases where there's more than maxBadConnRetries bad connections in
the
+// pool (issue 8834)
+func TestManyErrBadConn(t *testing.T) {
+ manyErrBadConnSetup := func() *DB {
+ db := newTestDB(t, "people")
+
+ nconn := maxBadConnRetries + 1
+ db.SetMaxIdleConns(nconn)
+ db.SetMaxOpenConns(nconn)
+ // open enough connections
+ func() {
+ for i := 0; i < nconn; i++ {
+ rows, err := db.Query("SELECT|people|age,name|")
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer rows.Close()
+ }
+ }()
+
+ if db.numOpen != nconn {
+ t.Fatalf("unexpected numOpen %d (was expecting %d)", db.numOpen, nconn)
+ } else if len(db.freeConn) != nconn {
+ t.Fatalf("unexpected len(db.freeConn) %d (was expecting %d)",
len(db.freeConn), nconn)
+ }
+ for _, conn := range db.freeConn {
+ conn.ci.(*fakeConn).stickyBad = true
+ }
+ return db
+ }
+
+ // Query
+ db := manyErrBadConnSetup()
+ defer closeDB(t, db)
+ rows, err := db.Query("SELECT|people|age,name|")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err = rows.Close(); err != nil {
+ t.Fatal(err)
+ }
+
+ // Exec
+ db = manyErrBadConnSetup()
+ defer closeDB(t, db)
+ _, err = db.Exec("INSERT|people|name=Julia,age=19")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ // Begin
+ db = manyErrBadConnSetup()
+ defer closeDB(t, db)
+ tx, err := db.Begin()
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err = tx.Rollback(); err != nil {
+ t.Fatal(err)
+ }
+
+ // Prepare
+ db = manyErrBadConnSetup()
+ defer closeDB(t, db)
+ stmt, err := db.Prepare("SELECT|people|age,name|")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err = stmt.Close(); err != nil {
+ t.Fatal(err)
+ }
+}
+
// golang.org/issue/5781
func TestErrBadConnReconnect(t *testing.T) {
db := newTestDB(t, "foo")

--
https://go-review.googlesource.com/2034

[Mikio Hara (Gerrit)]

Mikio Hara has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 1:

is this still alive? if not, pls abandon.

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Mikio Hara <mikioh...@gmail.com>
Gerrit-HasComments: No

[Marko Tiikkaja (Gerrit)]

Marko Tiikkaja has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 1:

> is this still alive? if not, pls abandon.

I very much expect to see this or something similar committed, so I guess
it's "alive", yes.

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>

[Marko Tiikkaja (Gerrit)]

Marko Tiikkaja uploaded a new patch set:

https://go-review.googlesource.com/2034

database/sql: Retry with a fresh connection after maxBadConnRetries

Previously if the connection pool was larger than maxBadConnRetries and
there were a lot of bad connections in the pool (for example if the
database server was restarted), a query might have failed with an
ErrBadConn unnecessarily. Instead of trying to guess how many times to
retry, try maxBadConnRetries times and then force a fresh connection to
be used for the last attempt. At the same time, lower maxBadConnRetries
to a smaller value now that it's not that important to retry so many
times from the free connection list.

Fixes #8834

Change-Id: I6542f151a766a658980fb396fa4880ecf5874e3d
---
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go
3 files changed, 138 insertions(+), 39 deletions(-)

[Mikio Hara (Gerrit)]

Mikio Hara has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 1:

thanks, but your cls to databease/sql apper to be conflicetd. is there any
good way to land a series of cls?

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>
Gerrit-Reviewer: Mikio Hara <mikioh...@gmail.com>

Gerrit-HasComments: No

[Marko Tiikkaja]

On 2015-01-16 10:08, 'Mikio Hara (Gerrit)' via golang-codereviews wrote:
> Mikio Hara has posted comments on this change.
>
> database/sql: Retry with a fresh connection after maxBadConnRetries
>
> Patch Set 1:
>
> thanks, but your cls to databease/sql apper to be conflicetd. is there any
> good way to land a series of cls?

I'm not sure what exactly you're asking.

They all conflict, because it's impossible to make them not conflict
without throwing them all into one huge CL. But I don't think that's a
problem. I can start rebasing them once they start going in, but so far
nothing has happened.


.marko

[Marko Tiikkaja (Gerrit)]

Marko Tiikkaja has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 2:

Ping?

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 2:

Can you remove the "false" and "true" literals from the call sites? bool
parameters almost always make code hard to read since they magically change
behavior and don't describe much.

Some named type with an iota of modes would be better. It could be:

type newConnStrategy uint8

const (
newConnAlways newConnStrategy = iota
cachedOrNew
)

Or something like that.

I only skimmed the code, so I don't exactly know what you need yet, but
that should make the code much more readable.

[Marko Tiikkaja (Gerrit)]

Marko Tiikkaja uploaded a new patch set:

https://go-review.googlesource.com/2034

database/sql: Retry with a fresh connection after maxBadConnRetries

Previously if the connection pool was larger than maxBadConnRetries
and there were a lot of bad connections in the pool (for example if
the database server was restarted), a query might have failed with an
ErrBadConn unnecessarily. Instead of trying to guess how many times
to retry, try maxBadConnRetries times and then force a fresh
connection to be used for the last attempt. At the same time, lower
maxBadConnRetries to a smaller value now that it's not that important
to retry so many times from the free connection list.

Fixes #8834

Change-Id: I6542f151a766a658980fb396fa4880ecf5874e3d
---
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go

3 files changed, 140 insertions(+), 39 deletions(-)

[Chris Bandy (Gerrit)]

Chris Bandy has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 3: Code-Review+1

LGTM

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Chris Bandy <bandy...@gmail.com>

Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>
Gerrit-Reviewer: Mikio Hara <mikioh...@gmail.com>

Gerrit-HasComments: No

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 3:

(7 comments)

Sorry for the slow review. I'm trying to catch up.

https://go-review.googlesource.com/#/c/2034/3/src/database/sql/sql.go
File src/database/sql/sql.go:

Line 238: // Determines whether connections can be reused in *DB.conn
Use complete Go-style sentences in docs:

// connReuseStrategy determines how (*DB).conn returns database connections.


Line 239: type connReuseStrategy int
type bool if there are only two options? or uint8? still use == below, and
don't treat it as a boolean.


Line 242: alwaysNewConn connReuseStrategy = iota
// alwaysNewConn forces a new connection to the database.


Line 243: cachedOrNewConn
// cachedOrNewConn returns a cached connection, if available, else
// creates a new database connection.

(or whatever the actual semantics are)


Line 647: func (db *DB) conn(ncs connReuseStrategy) (*driverConn, error) {
"ncs" doesn't mean anything to me, or most readers. Let's say:

func (db *DB) conn(strategy connReuseStategy) ...


Line 824: func (db *DB) prepare(query string, ncs connReuseStrategy)
(*Stmt, error) {
strategy instead of ncs


Line 870: func (db *DB) exec(query string, args []interface{}, ncs
connReuseStrategy) (res Result, err error) {
strategy

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Chris Bandy <bandy...@gmail.com>
Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>
Gerrit-Reviewer: Mikio Hara <mikioh...@gmail.com>

Gerrit-HasComments: Yes

[Marko Tiikkaja (Gerrit)]

Reviewers: Chris Bandy

Marko Tiikkaja uploaded a new patch set:

https://go-review.googlesource.com/2034

database/sql: Retry with a fresh connection after maxBadConnRetries

Previously if the connection pool was larger than maxBadConnRetries
and there were a lot of bad connections in the pool (for example if
the database server was restarted), a query might have failed with an
ErrBadConn unnecessarily. Instead of trying to guess how many times
to retry, try maxBadConnRetries times and then force a fresh
connection to be used for the last attempt. At the same time, lower
maxBadConnRetries to a smaller value now that it's not that important
to retry so many times from the free connection list.

Fixes #8834

Change-Id: I6542f151a766a658980fb396fa4880ecf5874e3d
---
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go

3 files changed, 147 insertions(+), 42 deletions(-)

[Marko Tiikkaja (Gerrit)]

Marko Tiikkaja has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 4:

> (7 comments)
>
> Sorry for the slow review. I'm trying to catch up.

No worries.

I've updated this according to your comments. PTAL.

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Chris Bandy <bandy...@gmail.com>
Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>
Gerrit-Reviewer: Mikio Hara <mikioh...@gmail.com>

Gerrit-HasComments: No

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 5: Run-TryBot+1

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Chris Bandy <bandy...@gmail.com>
Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 5:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=766cc567

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Chris Bandy <bandy...@gmail.com>

Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 5: Code-Review+2

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has submitted this change and it was merged.

database/sql: Retry with a fresh connection after maxBadConnRetries

Previously if the connection pool was larger than maxBadConnRetries
and there were a lot of bad connections in the pool (for example if
the database server was restarted), a query might have failed with an
ErrBadConn unnecessarily. Instead of trying to guess how many times
to retry, try maxBadConnRetries times and then force a fresh
connection to be used for the last attempt. At the same time, lower
maxBadConnRetries to a smaller value now that it's not that important
to retry so many times from the free connection list.

Fixes #8834

Change-Id: I6542f151a766a658980fb396fa4880ecf5874e3d
Reviewed-on: https://go-review.googlesource.com/2034
Run-TryBot: Brad Fitzpatrick <brad...@golang.org>
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>

---
M src/database/sql/fakedb_test.go
M src/database/sql/sql.go
M src/database/sql/sql_test.go

3 files changed, 147 insertions(+), 42 deletions(-)

Approvals:
Brad Fitzpatrick: Looks good to me, approved; Run TryBots

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

database/sql: Retry with a fresh connection after maxBadConnRetries

Patch Set 5:

TryBots are happy.

--
https://go-review.googlesource.com/2034
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Chris Bandy <bandy...@gmail.com>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Marko Tiikkaja <ma...@joh.to>

Gerrit-HasComments: No