From c7fa0af7b572bfac8d17cca6be34f8176b2d26fa Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Wed, 14 May 2025 14:57:52 -0700 Subject: [PATCH] Bug fix: ensure an active transaction is set before preparing a query Also adds support for running transaction tests with prepared statements. --- engine.go | 3 ++ enginetest/evaluation.go | 58 ++++++++++++++++++----- enginetest/queries/transaction_queries.go | 31 ++++++++++++ 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/engine.go b/engine.go index e85fb1e8dd..9b7faf4f7a 100644 --- a/engine.go +++ b/engine.go @@ -240,6 +240,9 @@ func (e *Engine) PrepareParsedQuery( statementKey, query string, stmt sqlparser.Statement, ) (sql.Node, error) { + // Make sure there is an active transaction if one hasn't been started yet + e.beginTransaction(ctx) + binder := planbuilder.New(ctx, e.Analyzer.Catalog, e.EventScheduler, e.Parser) node, _, err := binder.BindOnly(stmt, query, nil) diff --git a/enginetest/evaluation.go b/enginetest/evaluation.go index 776b60e1d6..4c2678a058 100644 --- a/enginetest/evaluation.go +++ b/enginetest/evaluation.go @@ -269,12 +269,23 @@ func TestTransactionScript(t *testing.T, harness Harness, script queries.Transac harness.Setup(setup.MydbData) e := mustNewEngine(t, harness) defer e.Close() - TestTransactionScriptWithEngine(t, e, harness, script) + TestTransactionScriptWithEngine(t, e, harness, script, false) }) } -// TestTransactionScriptWithEngine runs the transaction test script given with the engine provided. -func TestTransactionScriptWithEngine(t *testing.T, e QueryEngine, harness Harness, script queries.TransactionTest) { +// TestTransactionScriptPrepared runs the test script given using prepared statements. +func TestTransactionScriptPrepared(t *testing.T, harness Harness, script queries.TransactionTest) bool { + return t.Run(script.Name, func(t *testing.T) { + harness.Setup(setup.MydbData) + e := mustNewEngine(t, harness) + defer e.Close() + TestTransactionScriptWithEngine(t, e, harness, script, true) + }) +} + +// TestTransactionScriptWithEngine runs the transaction test script given with the engine provided. If |prepared| is true +// then the queries will be prepared and then executed, otherwise they will be executed directly. +func TestTransactionScriptWithEngine(t *testing.T, e QueryEngine, harness Harness, script queries.TransactionTest, prepared bool) { setupSession := NewSession(harness) for _, statement := range script.SetUpScript { if sh, ok := harness.(SkippingHarness); ok { @@ -307,17 +318,40 @@ func TestTransactionScriptWithEngine(t *testing.T, e QueryEngine, harness Harnes } if assertion.ExpectedErr != nil { - AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, assertion.ExpectedErr) + if prepared { + AssertErrPreparedWithCtx(t, e, harness, clientSession, assertion.Query, assertion.ExpectedErr) + } else { + AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, assertion.ExpectedErr) + } } else if assertion.ExpectedErrStr != "" { - AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, nil, assertion.ExpectedErrStr) + if prepared { + AssertErrPreparedWithCtx(t, e, harness, clientSession, assertion.Query, nil, assertion.ExpectedErrStr) + } else { + AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, nil, assertion.ExpectedErrStr) + } } else if assertion.ExpectedWarning != 0 { - AssertWarningAndTestQuery(t, e, nil, harness, assertion.Query, assertion.Expected, - nil, assertion.ExpectedWarning, assertion.ExpectedWarningsCount, - assertion.ExpectedWarningMessageSubstring, false) + if prepared { + // TODO: Looks like we don't have a prepared version of this yet + AssertWarningAndTestQuery(t, e, nil, harness, assertion.Query, assertion.Expected, + nil, assertion.ExpectedWarning, assertion.ExpectedWarningsCount, + assertion.ExpectedWarningMessageSubstring, false) + } else { + AssertWarningAndTestQuery(t, e, nil, harness, assertion.Query, assertion.Expected, + nil, assertion.ExpectedWarning, assertion.ExpectedWarningsCount, + assertion.ExpectedWarningMessageSubstring, false) + } } else if assertion.SkipResultsCheck { - RunQueryWithContext(t, e, harness, clientSession, assertion.Query) + if prepared { + runQueryPreparedWithCtx(t, clientSession, e, assertion.Query, assertion.Bindings, false) + } else { + RunQueryWithContext(t, e, harness, clientSession, assertion.Query) + } } else { - TestQueryWithContext(t, clientSession, e, harness, assertion.Query, assertion.Expected, nil, nil, nil) + if prepared { + TestPreparedQueryWithContext(t, clientSession, e, harness, assertion.Query, assertion.Expected, nil, nil, false) + } else { + TestQueryWithContext(t, clientSession, e, harness, assertion.Query, assertion.Expected, nil, nil, nil) + } } }) } @@ -551,7 +585,7 @@ func injectBindVarsAndPrepare( switch p := parsed.(type) { case *sqlparser.Load, *sqlparser.Prepare, *sqlparser.Execute: - // LOAD DATA query cannot be used as PREPARED STATEMENT + // LOAD DATA, PREPARE, and EXECUTE queries cannot be used as prepared statements return q, nil, nil case *sqlparser.Set: // SET system variable query cannot be used as PREPARED STATEMENT @@ -564,7 +598,7 @@ func injectBindVarsAndPrepare( b := planbuilder.New(ctx, e.EngineAnalyzer().Catalog, e.EngineEventScheduler(), nil) b.SetParserOptions(sql.LoadSqlMode(ctx).ParserOptions()) - resPlan, _, err := b.BindOnly(parsed, q, nil) + resPlan, err := e.PrepareQuery(ctx, q) if err != nil { return q, nil, err } diff --git a/enginetest/queries/transaction_queries.go b/enginetest/queries/transaction_queries.go index 77a68e381e..bdc1fb753a 100644 --- a/enginetest/queries/transaction_queries.go +++ b/enginetest/queries/transaction_queries.go @@ -1447,4 +1447,35 @@ var TransactionTests = []TransactionTest{ }, }, }, + { + // Repro for https://github.com/dolthub/dolt/issues/3402 + // and https://github.com/dolthub/dolt/issues/9213 (similar issue, just with prepared statements) + Name: "DDL changes from transactions are available before analyzing statements in other sessions (autocommit on)", + Assertions: []ScriptTestAssertion{ + { + Query: "/* client a */ select @@autocommit;", + Expected: []sql.Row{{1}}, + }, + { + Query: "/* client b */ select @@autocommit;", + Expected: []sql.Row{{1}}, + }, + { + Query: "/* client a */ show tables like 't';", + Expected: []sql.Row{}, + }, + { + Query: "/* client b */ show tables like 't';", + Expected: []sql.Row{}, + }, + { + Query: "/* client a */ create table t(pk int primary key);", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + Query: "/* client b */ select count(*) from t;", + Expected: []sql.Row{{0}}, + }, + }, + }, }