diff --git a/engine.go b/engine.go index e56aea8f7e..888fbe0be4 100644 --- a/engine.go +++ b/engine.go @@ -19,7 +19,7 @@ import ( "os" "sync" - "github.com/dolthub/go-mysql-server/sql/transform" + "github.com/pkg/errors" "github.com/dolthub/go-mysql-server/memory" "github.com/dolthub/go-mysql-server/sql" @@ -29,6 +29,7 @@ import ( "github.com/dolthub/go-mysql-server/sql/grant_tables" "github.com/dolthub/go-mysql-server/sql/parse" "github.com/dolthub/go-mysql-server/sql/plan" + "github.com/dolthub/go-mysql-server/sql/transform" ) // Config for the Engine. @@ -174,17 +175,29 @@ func (e *Engine) QueryNodeWithBindings( err error ) + if parsed == nil { + parsed, err = parse.Parse(ctx, query) + if err != nil { + return nil, nil, err + } + } + + _, err = e.beginTransaction(ctx, parsed) + if err != nil { + return nil, nil, err + } + if p, ok := e.preparedDataForSession(ctx.Session); ok && p.Query == query { analyzed, err = e.analyzePreparedQuery(ctx, query, bindings) } else { analyzed, err = e.analyzeQuery(ctx, query, parsed, bindings) } if err != nil { - return nil, nil, err - } + err2 := clearAutocommitTransaction(ctx) + if err2 != nil { + err = errors.Wrap(err, "unable to clear autocommit transaction: "+err2.Error()) + } - _, err = e.beginTransaction(ctx, analyzed) - if err != nil { return nil, nil, err } @@ -200,6 +213,11 @@ func (e *Engine) QueryNodeWithBindings( iter, err = analyzed.RowIter(ctx, nil) } if err != nil { + err2 := clearAutocommitTransaction(ctx) + if err2 != nil { + err = errors.Wrap(err, "unable to clear autocommit transaction: "+err2.Error()) + } + return nil, nil, err } @@ -214,6 +232,32 @@ func (e *Engine) QueryNodeWithBindings( return analyzed.Schema(), iter, nil } +// clearAutocommitTransaction unsets the transaction from the current session if it is an implicitly +// created autocommit transaction. This enables the next request to have an autocommit transaction +// correctly started. +func clearAutocommitTransaction(ctx *sql.Context) error { + // The GetIgnoreAutoCommit property essentially says the current transaction is an explicit, + // user-created transaction and we should not process autocommit. So, if it's set, then we + // don't need to do anything here to clear implicit transaction state. + // + // TODO: This logic would probably read more clearly if we could just ask the session/ctx if the + // current transaction is automatically created or explicitly created by the caller. + if ctx.GetIgnoreAutoCommit() { + return nil + } + + autocommit, err := plan.IsSessionAutocommit(ctx) + if err != nil { + return err + } + + if autocommit { + ctx.SetTransaction(nil) + } + + return nil +} + func (e *Engine) cachePreparedStmt(ctx *sql.Context, analyzed sql.Node, query string) { e.mu.Lock() defer e.mu.Unlock() diff --git a/enginetest/transaction_queries.go b/enginetest/transaction_queries.go index 3e2857d6c4..ce750c3a02 100755 --- a/enginetest/transaction_queries.go +++ b/enginetest/transaction_queries.go @@ -34,6 +34,57 @@ type TransactionTest struct { } var TransactionTests = []TransactionTest{ + { + Name: "Changes from transactions are available before analyzing statements in other sessions (autocommit off)", + Assertions: []ScriptTestAssertion{ + { + Query: "/* client a */ set @@autocommit = 0;", + Expected: []sql.Row{{}}, + }, + { + Query: "/* client b */ set @@autocommit = 0;", + Expected: []sql.Row{{}}, + }, + { + Query: "/* client a */ select @@autocommit;", + Expected: []sql.Row{{0}}, + }, + { + Query: "/* client b */ select @@autocommit;", + Expected: []sql.Row{{0}}, + }, + { + Query: "/* client a */ start transaction;", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ select * from t;", + ExpectedErr: sql.ErrTableNotFound, + }, + { + Query: "/* client a */ create table t(pk int primary key);", + Expected: []sql.Row{{sql.OkResult{}}}, + }, + { + // Trigger a query error to make sure explicit transaction is still + // correctly configured in the session despite the error + Query: "/* client a */ select * from t2;", + ExpectedErr: sql.ErrTableNotFound, + }, + { + Query: "/* client a */ commit;", + Expected: []sql.Row{}, + }, + { + Query: "/* client b */ start transaction;", + Expected: []sql.Row{}, + }, + { + Query: "/* client b */ select count(*) from t;", + Expected: []sql.Row{{0}}, + }, + }, + }, { Name: "autocommit on", SetUpScript: []string{ @@ -184,6 +235,11 @@ var TransactionTests = []TransactionTest{ Query: "/* client a */ start transaction", Expected: []sql.Row{}, }, + { + // Trigger an analyzer error to make sure transaction state is managed correctly + Query: "/* client a */ select * from doesnotexist;", + ExpectedErr: sql.ErrTableNotFound, + }, { Query: "/* client a */ insert into t values (2, 2)", Expected: []sql.Row{{sql.NewOkResult(1)}}, @@ -192,6 +248,12 @@ var TransactionTests = []TransactionTest{ Query: "/* client b */ select * from t order by x", Expected: []sql.Row{{1, 1}}, }, + { + // Trigger an analyzer error to make sure state for the explicitly started + // transaction is managed correctly and not cleared + Query: "/* client a */ select * from doesnotexist;", + ExpectedErr: sql.ErrTableNotFound, + }, { Query: "/* client a */ commit", Expected: []sql.Row{}, diff --git a/sql/plan/transaction_committing_iter.go b/sql/plan/transaction_committing_iter.go index 69d760e7dc..b351a7bcca 100644 --- a/sql/plan/transaction_committing_iter.go +++ b/sql/plan/transaction_committing_iter.go @@ -122,10 +122,10 @@ func (t transactionCommittingIter) Close(ctx *sql.Context) error { } tx := ctx.GetTransaction() - // TODO: In the future we should ensure that analyzer supports impicit commits instead of directly + // TODO: In the future we should ensure that analyzer supports implicit commits instead of directly // accessing autocommit here. // cc. https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html - autocommit, err := isSessionAutocommit(ctx) + autocommit, err := IsSessionAutocommit(ctx) if err != nil { return err } @@ -144,7 +144,9 @@ func (t transactionCommittingIter) Close(ctx *sql.Context) error { return nil } -func isSessionAutocommit(ctx *sql.Context) (bool, error) { +// IsSessionAutocommit returns true if the current session is using implicit transaction management +// through autocommit. +func IsSessionAutocommit(ctx *sql.Context) (bool, error) { if ReadCommitted(ctx) { return true, nil }