Skip to content

Commit

Permalink
[release-branch.go1.13] database/sql: backport 5 Tx rollback related CLs
Browse files Browse the repository at this point in the history
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.13.13 that weren't in Go1.13, 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 #40205

Change-Id: I6d7307180b0db0bf159130d91161764cf0f18b58
Reviewed-on: https://go-review.googlesource.com/c/go/+/242522
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Theophanes <kardianos@gmail.com>
  • Loading branch information
odeke-em authored and andybons committed Jul 16, 2020
1 parent 6e1b89a commit 42e7927
Show file tree
Hide file tree
Showing 4 changed files with 375 additions and 98 deletions.
9 changes: 3 additions & 6 deletions src/database/sql/driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,12 +255,9 @@ type ConnBeginTx interface {
// SessionResetter may be implemented by Conn to allow drivers to reset the
// session state associated with the connection and to signal a bad connection.
type SessionResetter interface {
// ResetSession is called while a connection is in the connection
// pool. No queries will run on this connection until this method returns.
//
// If the connection is bad this should return driver.ErrBadConn to prevent
// the connection from being returned to the connection pool. Any other
// error will be discarded.
// ResetSession is called prior to executing a query on the connection
// if the connection has been used before. If the driver returns ErrBadConn
// the connection is discarded.
ResetSession(ctx context.Context) error
}

Expand Down
45 changes: 43 additions & 2 deletions src/database/sql/fakedb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,12 +393,19 @@ func setStrictFakeConnClose(t *testing.T) {

func (c *fakeConn) ResetSession(ctx context.Context) error {
c.dirtySession = false
c.currTx = nil
if c.isBad() {
return driver.ErrBadConn
}
return nil
}

var _ validator = (*fakeConn)(nil)

func (c *fakeConn) IsValid() bool {
return !c.isBad()
}

func (c *fakeConn) Close() (err error) {
drv := fdriver.(*fakeDriver)
defer func() {
Expand Down Expand Up @@ -731,6 +738,9 @@ var hookExecBadConn func() bool
func (s *fakeStmt) Exec(args []driver.Value) (driver.Result, error) {
panic("Using ExecContext")
}

var errFakeConnSessionDirty = errors.New("fakedb: session is dirty")

func (s *fakeStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (driver.Result, error) {
if s.panic == "Exec" {
panic(s.panic)
Expand All @@ -743,7 +753,7 @@ func (s *fakeStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (d
return nil, driver.ErrBadConn
}
if s.c.isDirtyAndMark() {
return nil, errors.New("fakedb: session is dirty")
return nil, errFakeConnSessionDirty
}

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

err := checkSubsetTypes(s.c.db.allowAny, args)
Expand Down Expand Up @@ -890,6 +900,37 @@ func (s *fakeStmt) QueryContext(ctx context.Context, args []driver.NamedValue) (
}
}
}
if s.table == "tx_status" && s.colName[0] == "tx_status" {
txStatus := "autocommit"
if s.c.currTx != nil {
txStatus = "transaction"
}
cursor := &rowsCursor{
parentMem: s.c,
posRow: -1,
rows: [][]*row{
[]*row{
{
cols: []interface{}{
txStatus,
},
},
},
},
cols: [][]string{
[]string{
"tx_status",
},
},
colType: [][]string{
[]string{
"string",
},
},
errPos: -1,
}
return cursor, nil
}

t.mu.Lock()

Expand Down
Loading

0 comments on commit 42e7927

Please sign in to comment.