Merge pull request #1013 from dolthub/fulghum/dolt-3402

Moving transaction initialization before query analysis

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()

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{},

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
 	}