[go] database/sql: simplify retry logic when got bad connection
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
database/sql: simplify retry logic when got bad connection
Simplify retry logic when got bad connection
Change-Id: I92494c6c020576ec01bc4868334ee920ded7aa57
GitHub-Last-Rev: 58acbceedea74ea57d36b26f2ceb238deda204d9
GitHub-Pull-Request: golang/go#54043
---
M src/database/sql/sql.go
1 file changed, 79 insertions(+), 96 deletions(-)
diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 854a895..4f08ee4 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -846,17 +846,12 @@
func (db *DB) PingContext(ctx context.Context) error {
var dc *driverConn
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- dc, err = db.conn(ctx, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- dc, err = db.conn(ctx, alwaysNewConn)
- }
+
+ db.retry(func(strategy connReuseStrategy) error {
+ dc, err = db.conn(ctx, strategy)
+ return err
+ })
+
if err != nil {
return err
}
@@ -1539,6 +1534,21 @@
// connection to be opened.
const maxBadConnRetries = 2
+func (db *DB) retry(fn func(strategy connReuseStrategy) error) error {
+ for i := int64(0); i < maxBadConnRetries; i++ {
+ err := fn(cachedOrNewConn)
+ // retry if err is driver.ErrBadConn
+ if err == nil || !errors.Is(err, driver.ErrBadConn) {
+ return err
+ }
+ }
+
+ if maxBadConnRetries > 0 {
+ return fn(alwaysNewConn)
+ }
+ return fn(cachedOrNewConn)
+}
+
// PrepareContext creates a prepared statement for later queries or executions.
// Multiple queries or executions may be run concurrently from the
// returned statement.
@@ -1550,17 +1560,12 @@
func (db *DB) PrepareContext(ctx context.Context, query string) (*Stmt, error) {
var stmt *Stmt
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- stmt, err = db.prepare(ctx, query, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.prepare(ctx, query, alwaysNewConn)
- }
+
+ db.retry(func(strategy connReuseStrategy) error {
+ stmt, err = db.prepare(ctx, query, strategy)
+ return err
+ })
+
return stmt, err
}
@@ -1628,17 +1633,11 @@
func (db *DB) ExecContext(ctx context.Context, query string, args ...any) (Result, error) {
var res Result
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- res, err = db.exec(ctx, query, args, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.exec(ctx, query, args, alwaysNewConn)
- }
+ db.retry(func(strategy connReuseStrategy) error {
+ res, err = db.exec(ctx, query, args, strategy)
+ return err
+ })
+
return res, err
}
@@ -1703,17 +1702,12 @@
func (db *DB) QueryContext(ctx context.Context, query string, args ...any) (*Rows, error) {
var rows *Rows
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- rows, err = db.query(ctx, query, args, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.query(ctx, query, args, alwaysNewConn)
- }
+
+ db.retry(func(strategy connReuseStrategy) error {
+ rows, err = db.query(ctx, query, args, strategy)
+ return err
+ })
+
return rows, err
}
@@ -1840,17 +1834,12 @@
func (db *DB) BeginTx(ctx context.Context, opts *TxOptions) (*Tx, error) {
var tx *Tx
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- tx, err = db.begin(ctx, opts, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.begin(ctx, opts, alwaysNewConn)
- }
+
+ db.retry(func(strategy connReuseStrategy) error {
+ tx, err = db.begin(ctx, opts, strategy)
+ return err
+ })
+
return tx, err
}
@@ -1921,17 +1910,12 @@
func (db *DB) Conn(ctx context.Context) (*Conn, error) {
var dc *driverConn
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- dc, err = db.conn(ctx, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- dc, err = db.conn(ctx, alwaysNewConn)
- }
+
+ db.retry(func(strategy connReuseStrategy) error {
+ dc, err = db.conn(ctx, strategy)
+ return err
+ })
+
if err != nil {
return nil, err
}
@@ -2620,26 +2604,18 @@
defer s.closemu.RUnlock()
var res Result
- strategy := cachedOrNewConn
- for i := 0; i < maxBadConnRetries+1; i++ {
- if i == maxBadConnRetries {
- strategy = alwaysNewConn
- }
+ err := s.db.retry(func(strategy connReuseStrategy) error {
dc, releaseConn, ds, err := s.connStmt(ctx, strategy)
if err != nil {
- if errors.Is(err, driver.ErrBadConn) {
- continue
- }
- return nil, err
+ return err
}
res, err = resultFromStatement(ctx, dc.ci, ds, args...)
releaseConn(err)
- if !errors.Is(err, driver.ErrBadConn) {
- return res, err
- }
- }
- return nil, driver.ErrBadConn
+ return err
+ })
+
+ return res, err
}
// Exec executes a prepared statement with the given arguments and
@@ -2768,24 +2744,19 @@
defer s.closemu.RUnlock()
var rowsi driver.Rows
- strategy := cachedOrNewConn
- for i := 0; i < maxBadConnRetries+1; i++ {
- if i == maxBadConnRetries {
- strategy = alwaysNewConn
- }
+ var rows *Rows
+
+ err := s.db.retry(func(strategy connReuseStrategy) error {
dc, releaseConn, ds, err := s.connStmt(ctx, strategy)
if err != nil {
- if errors.Is(err, driver.ErrBadConn) {
- continue
- }
- return nil, err
+ return err
}
rowsi, err = rowsiFromStatement(ctx, dc.ci, ds, args...)
if err == nil {
// Note: ownership of ci passes to the *Rows, to be freed
// with releaseConn.
- rows := &Rows{
+ rows = &Rows{
dc: dc,
rowsi: rowsi,
// releaseConn set below
@@ -2805,15 +2776,14 @@
txctx = s.cg.txCtx()
}
rows.initContextClose(ctx, txctx)
- return rows, nil
+ return nil
}
releaseConn(err)
- if !errors.Is(err, driver.ErrBadConn) {
- return nil, err
- }
- }
- return nil, driver.ErrBadConn
+ return err
+ })
+
+ return rows, err
}
// Query executes a prepared query statement with the given arguments
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Theophanes (Gerrit)
1 comment:
Patchset:
Right now you are inconsistent with whether you use the error that retry returns or not. If you return an error, please use it. If you don't don't return it, please comment the error must be captured within the closure.
I think your current code is correct, it is just inconsistent.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Daniel Theophanes.
1 comment:
Patchset:
Right now you are inconsistent with whether you use the error that retry returns or not. […]
Changed to use the returned error.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Daniel Theophanes.
Gerrit Bot uploaded patch set #2 to this change.
database/sql: simplify retry logic when got bad connection
Simplify retry logic when got bad connection
Change-Id: I92494c6c020576ec01bc4868334ee920ded7aa57
GitHub-Last-Rev: a3848947b9407a20248e9d1cd04eec8839c2adf1
GitHub-Pull-Request: golang/go#54043
---
M src/database/sql/sql.go
1 file changed, 80 insertions(+), 96 deletions(-)
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Theophanes (Gerrit)
Patch set 2:Run-TryBot +1
Daniel Theophanes (Gerrit)
Patch set 2:Code-Review +2
1 comment:
Patchset:
I like how this simplifies the bad conn retry logic. It also simplifies a patch to remove it or make it configurable in a fork or in the future.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Patch set 2:Code-Review +1
2 comments:
Patchset:
Thanks for the CL. We're still in the Go 1.19 release freeze. This needs to wait for the next release.
File src/database/sql/sql.go:
Patch Set #2, Line 1549:
return fn(cachedOrNewConn)This line is never reached as the constant maxBadConnRetries is not 0. Or the purpose for this line is to handle the case maxBadConnRetries is 0 (say, if it changes in the future)? If this is the case, it may be cleaner to write it separately at the top, something like
if maxBadConnRetries == 0 { return fn(cachedOrNewConn) }
for i := int64(0);... { ... }
return fn(alwaysNewConn)?
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Cherry Mui, Daniel Theophanes.
1 comment:
File src/database/sql/sql.go:
Patch Set #2, Line 1549:
return fn(cachedOrNewConn)
This line is never reached as the constant maxBadConnRetries is not 0. […]
Good catch, the `maxBadConnRetries` was allowed to be configured in the original PR.
So should we just remove the > 0 check? or we can change the `maxBadConnRetries` from `const` to `var`, which allow developers change its value with `go:linkname`
cc @Daniel Theophanes
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Mui (Gerrit)
Attention is currently required from: Daniel Theophanes, Jinzhu Zhang.
1 comment:
File src/database/sql/sql.go:
Patch Set #2, Line 1549:
return fn(cachedOrNewConn)
I think it is fine to remove the >0 check.
or we can change the maxBadConnRetries from const to var , which allow developers change its value with go:linkname
Please don't. Using linkname to change a non-exported variable is VERY BAD practice. We should not do that. Thanks.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Cherry Mui, Daniel Theophanes.
1 comment:
File src/database/sql/sql.go:
Patch Set #2, Line 1549:
return fn(cachedOrNewConn)
I think it is fine to remove the >0 check. […]
removed the >0 check.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Cherry Mui, Daniel Theophanes.
Gerrit Bot uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Daniel Theophanes, TryBot-Result+1 by Gopher Robot
database/sql: simplify retry logic when got bad connection
Simplify retry logic when got bad connection
Change-Id: I92494c6c020576ec01bc4868334ee920ded7aa57
GitHub-Last-Rev: 7499b0c9419a31c9adce6d5096a1924aa3612f1d
GitHub-Pull-Request: golang/go#54043
---
M src/database/sql/sql.go
1 file changed, 77 insertions(+), 96 deletions(-)
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Cherry Mui, Daniel Theophanes.
1 comment:
Patchset:
Hello @kardianos @cherryyz, is there any other feedback for this PR?
Thank you.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
hopehook (Gerrit)
Attention is currently required from: Cherry Mui, Daniel Theophanes.
Patch set 3:Run-TryBot +1
Benny Siegert (Gerrit)
Attention is currently required from: Cherry Mui, Daniel Theophanes.
Patch set 3:Code-Review +1
1 comment:
Commit Message:
Patch Set #3, Line 9:
Simplify retry logic when got bad connectionnit: when the connection is bad
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Jinzhu Zhang (Gerrit)
Attention is currently required from: Benny Siegert, Cherry Mui, Daniel Theophanes.
1 comment:
Commit Message:
Patch Set #3, Line 9:
Simplify retry logic when got bad connectionnit: when the connection is bad
In case of ErrBadConn happen? usually network problem.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Zeke Lu (Gerrit)
Attention is currently required from: Benny Siegert, Cherry Mui, Daniel Theophanes, Jinzhu Zhang.
1 comment:
Commit Message:
Patch Set #3, Line 9:
Simplify retry logic when got bad connection
In case of ErrBadConn happen? usually network problem.
I think Benny suggested to change the commit message to `Simplify retry logic when the connection is bad`.
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Benny Siegert, Cherry Mui, Daniel Theophanes, Jinzhu Zhang.
Gerrit Bot uploaded patch set #4 to this change.
database/sql: simplify retry logic when the connection is bad
Simplify retry logic when got bad connection
Change-Id: I92494c6c020576ec01bc4868334ee920ded7aa57
GitHub-Last-Rev: 7499b0c9419a31c9adce6d5096a1924aa3612f1d
GitHub-Pull-Request: golang/go#54043
---
M src/database/sql/sql.go
1 file changed, 77 insertions(+), 96 deletions(-)
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Theophanes (Gerrit)
Daniel Theophanes submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/database/sql/sql.go
Insertions: 1, Deletions: 4.
@@ -1543,10 +1543,7 @@
}
}
- if maxBadConnRetries > 0 {
- return fn(alwaysNewConn)
- }
- return fn(cachedOrNewConn)
+ return fn(alwaysNewConn)
}
// PrepareContext creates a prepared statement for later queries or executions.
```
database/sql: simplify retry logic when the connection is bad
Simplify retry logic when got bad connection
Change-Id: I92494c6c020576ec01bc4868334ee920ded7aa57
GitHub-Last-Rev: 7499b0c9419a31c9adce6d5096a1924aa3612f1d
GitHub-Pull-Request: golang/go#54043
Reviewed-on: https://go-review.googlesource.com/c/go/+/419182
Reviewed-by: Daniel Theophanes <kard...@gmail.com>
Reviewed-by: Cherry Mui <cher...@google.com>
Reviewed-by: Benny Siegert <bsie...@gmail.com>
Run-TryBot: hopehook <hope...@golangcn.org>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/database/sql/sql.go
1 file changed, 83 insertions(+), 96 deletions(-)
diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 854a895..e74dd87 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -846,17 +846,12 @@
func (db *DB) PingContext(ctx context.Context) error {
var dc *driverConn
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- dc, err = db.conn(ctx, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- dc, err = db.conn(ctx, alwaysNewConn)
- }
+
+ err = db.retry(func(strategy connReuseStrategy) error {
+ dc, err = db.conn(ctx, strategy)
+ return err
+ })
+
if err != nil {
return err
}
@@ -1539,6 +1534,18 @@
// connection to be opened.
const maxBadConnRetries = 2
+func (db *DB) retry(fn func(strategy connReuseStrategy) error) error {
+ for i := int64(0); i < maxBadConnRetries; i++ {
+ err := fn(cachedOrNewConn)
+ // retry if err is driver.ErrBadConn
+ if err == nil || !errors.Is(err, driver.ErrBadConn) {
+ return err
+ }
+ }
+
+ return fn(alwaysNewConn)
+}
+
// PrepareContext creates a prepared statement for later queries or executions.
// Multiple queries or executions may be run concurrently from the
// returned statement.
@@ -1550,17 +1557,12 @@
func (db *DB) PrepareContext(ctx context.Context, query string) (*Stmt, error) {
var stmt *Stmt
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- stmt, err = db.prepare(ctx, query, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.prepare(ctx, query, alwaysNewConn)
- }
+
+ err = db.retry(func(strategy connReuseStrategy) error {
+ stmt, err = db.prepare(ctx, query, strategy)
+ return err
+ })
+
return stmt, err
}
@@ -1628,17 +1630,12 @@
func (db *DB) ExecContext(ctx context.Context, query string, args ...any) (Result, error) {
var res Result
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- res, err = db.exec(ctx, query, args, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.exec(ctx, query, args, alwaysNewConn)
- }
+
+ err = db.retry(func(strategy connReuseStrategy) error {
+ res, err = db.exec(ctx, query, args, strategy)
+ return err
+ })
+
return res, err
}
@@ -1703,17 +1700,12 @@
func (db *DB) QueryContext(ctx context.Context, query string, args ...any) (*Rows, error) {
var rows *Rows
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- rows, err = db.query(ctx, query, args, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.query(ctx, query, args, alwaysNewConn)
- }
+
+ err = db.retry(func(strategy connReuseStrategy) error {
+ rows, err = db.query(ctx, query, args, strategy)
+ return err
+ })
+
return rows, err
}
@@ -1840,17 +1832,12 @@
func (db *DB) BeginTx(ctx context.Context, opts *TxOptions) (*Tx, error) {
var tx *Tx
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- tx, err = db.begin(ctx, opts, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- return db.begin(ctx, opts, alwaysNewConn)
- }
+
+ err = db.retry(func(strategy connReuseStrategy) error {
+ tx, err = db.begin(ctx, opts, strategy)
+ return err
+ })
+
return tx, err
}
@@ -1921,17 +1908,12 @@
func (db *DB) Conn(ctx context.Context) (*Conn, error) {
var dc *driverConn
var err error
- var isBadConn bool
- for i := 0; i < maxBadConnRetries; i++ {
- dc, err = db.conn(ctx, cachedOrNewConn)
- isBadConn = errors.Is(err, driver.ErrBadConn)
- if !isBadConn {
- break
- }
- }
- if isBadConn {
- dc, err = db.conn(ctx, alwaysNewConn)
- }
+
+ err = db.retry(func(strategy connReuseStrategy) error {
+ dc, err = db.conn(ctx, strategy)
+ return err
+ })
+
if err != nil {
return nil, err
}
@@ -2620,26 +2602,18 @@@@ -2768,24 +2742,19 @@@@ -2805,15 +2774,14 @@
txctx = s.cg.txCtx()
}
rows.initContextClose(ctx, txctx)
- return rows, nil
+ return nil
}
releaseConn(err)
- if !errors.Is(err, driver.ErrBadConn) {
- return nil, err
- }
- }
- return nil, driver.ErrBadConn
+ return err
+ })
+
+ return rows, err
}
// Query executes a prepared query statement with the given arguments
To view, visit change 419182. To unsubscribe, or for help writing mail filters, visit settings.