Skip to content

Commit 399ce80

Browse files
odeke-emandybons
authored andcommitted
[release-branch.go1.14] database/sql: backport 5 Tx rollback related CLs
Manually backported the subject CLs, because of lack of Gerrit "forge-author" permissions, but also because the prior cherry picks didn't apply cleanly, due to a tight relation chain. The backport comprises of: * CL 174122 * CL 216197 * CL 223963 * CL 216240 * CL 216241 Note: Due to the restrictions that we cannot retroactively introduce API changes to Go1.14.6 that weren't in Go1.14, the Conn.Validator interface (from CL 174122, CL 223963) isn't exposed, and drivers will just be inspected, for if they have an IsValid() bool method implemented. For a description of the content of each CL: * CL 174122: database/sql: process all Session Resets synchronously Adds a new interface, driver.ConnectionValidator, to allow drivers to signal they should not be used again, separatly from the session resetter interface. This is done now that the session reset is done after the connection is put into the connection pool. Previous behavior attempted to run Session Resets in a background worker. This implementation had two problems: untested performance gains for additional complexity, and failures when the pool size exceeded the connection reset channel buffer size. * CL 216197: database/sql: check conn expiry when returning to pool, not when handing it out With the original connection reuse strategy, it was possible that when a new connection was requested, the pool would wait for an an existing connection to return for re-use in a full connection pool, and then it would check if the returned connection was expired. If the returned connection expired while awaiting re-use, it would return an error to the location requestiong the new connection. The existing call sites requesting a new connection was often the last attempt at returning a connection for a query. This would then result in a failed query. This change ensures that we perform the expiry check right before a connection is inserted back in to the connection pool for while requesting a new connection. If requesting a new connection it will no longer fail due to the connection expiring. * CL 216240: database/sql: prevent Tx statement from committing after rollback It was possible for a Tx that was aborted for rollback asynchronously to execute a query after the rollback had completed on the database, which often would auto commit the query outside of the transaction. By W-locking the tx.closemu prior to issuing the rollback connection it ensures any Tx query either fails or finishes on the Tx, and never after the Tx has rolled back. * CL 216241: database/sql: on Tx rollback, retain connection if driver can reset session Previously the Tx would drop the connection after rolling back from a context cancel. Now if the driver can reset the session, keep the connection. * CL 223963 database/sql: add test for Conn.Validator interface This addresses comments made by Russ after https://golang.org/cl/174122 was merged. It addes a test for the connection validator and renames the interface to just "Validator". Updates #31480 Updates #32530 Updates #32942 Updates #34775 Fixes #39101 Change-Id: I043d2d724a367588689fd7d6f3cecb39abeb042c Reviewed-on: https://go-review.googlesource.com/c/go/+/242102 Run-TryBot: Emmanuel Odeke <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Daniel Theophanes <[email protected]>
1 parent bce174c commit 399ce80

File tree

4 files changed

+375
-98
lines changed

4 files changed

+375
-98
lines changed

src/database/sql/driver/driver.go

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -255,12 +255,9 @@ type ConnBeginTx interface {
255255
// SessionResetter may be implemented by Conn to allow drivers to reset the
256256
// session state associated with the connection and to signal a bad connection.
257257
type SessionResetter interface {
258-
// ResetSession is called while a connection is in the connection
259-
// pool. No queries will run on this connection until this method returns.
260-
//
261-
// If the connection is bad this should return driver.ErrBadConn to prevent
262-
// the connection from being returned to the connection pool. Any other
263-
// error will be discarded.
258+
// ResetSession is called prior to executing a query on the connection
259+
// if the connection has been used before. If the driver returns ErrBadConn
260+
// the connection is discarded.
264261
ResetSession(ctx context.Context) error
265262
}
266263

src/database/sql/fakedb_test.go

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -390,12 +390,19 @@ func setStrictFakeConnClose(t *testing.T) {
390390

391391
func (c *fakeConn) ResetSession(ctx context.Context) error {
392392
c.dirtySession = false
393+
c.currTx = nil
393394
if c.isBad() {
394395
return driver.ErrBadConn
395396
}
396397
return nil
397398
}
398399

400+
var _ validator = (*fakeConn)(nil)
401+
402+
func (c *fakeConn) IsValid() bool {
403+
return !c.isBad()
404+
}
405+
399406
func (c *fakeConn) Close() (err error) {
400407
drv := fdriver.(*fakeDriver)
401408
defer func() {
@@ -728,6 +735,9 @@ var hookExecBadConn func() bool
728735
func (s *fakeStmt) Exec(args []driver.Value) (driver.Result, error) {
729736
panic("Using ExecContext")
730737
}
738+
739+
var errFakeConnSessionDirty = errors.New("fakedb: session is dirty")
740+
731741
func (s *fakeStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (driver.Result, error) {
732742
if s.panic == "Exec" {
733743
panic(s.panic)
@@ -740,7 +750,7 @@ func (s *fakeStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (d
740750
return nil, driver.ErrBadConn
741751
}
742752
if s.c.isDirtyAndMark() {
743-
return nil, errors.New("fakedb: session is dirty")
753+
return nil, errFakeConnSessionDirty
744754
}
745755

746756
err := checkSubsetTypes(s.c.db.allowAny, args)
@@ -854,7 +864,7 @@ func (s *fakeStmt) QueryContext(ctx context.Context, args []driver.NamedValue) (
854864
return nil, driver.ErrBadConn
855865
}
856866
if s.c.isDirtyAndMark() {
857-
return nil, errors.New("fakedb: session is dirty")
867+
return nil, errFakeConnSessionDirty
858868
}
859869

860870
err := checkSubsetTypes(s.c.db.allowAny, args)
@@ -887,6 +897,37 @@ func (s *fakeStmt) QueryContext(ctx context.Context, args []driver.NamedValue) (
887897
}
888898
}
889899
}
900+
if s.table == "tx_status" && s.colName[0] == "tx_status" {
901+
txStatus := "autocommit"
902+
if s.c.currTx != nil {
903+
txStatus = "transaction"
904+
}
905+
cursor := &rowsCursor{
906+
parentMem: s.c,
907+
posRow: -1,
908+
rows: [][]*row{
909+
[]*row{
910+
{
911+
cols: []interface{}{
912+
txStatus,
913+
},
914+
},
915+
},
916+
},
917+
cols: [][]string{
918+
[]string{
919+
"tx_status",
920+
},
921+
},
922+
colType: [][]string{
923+
[]string{
924+
"string",
925+
},
926+
},
927+
errPos: -1,
928+
}
929+
return cursor, nil
930+
}
890931

891932
t.mu.Lock()
892933

0 commit comments

Comments
 (0)