Skip to content

Commit c7fa0af

Browse files
committed
Bug fix: ensure an active transaction is set before preparing a query
Also adds support for running transaction tests with prepared statements.
1 parent e116aa6 commit c7fa0af

File tree

3 files changed

+80
-12
lines changed

3 files changed

+80
-12
lines changed

engine.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,9 @@ func (e *Engine) PrepareParsedQuery(
240240
statementKey, query string,
241241
stmt sqlparser.Statement,
242242
) (sql.Node, error) {
243+
// Make sure there is an active transaction if one hasn't been started yet
244+
e.beginTransaction(ctx)
245+
243246
binder := planbuilder.New(ctx, e.Analyzer.Catalog, e.EventScheduler, e.Parser)
244247
node, _, err := binder.BindOnly(stmt, query, nil)
245248

enginetest/evaluation.go

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -269,12 +269,23 @@ func TestTransactionScript(t *testing.T, harness Harness, script queries.Transac
269269
harness.Setup(setup.MydbData)
270270
e := mustNewEngine(t, harness)
271271
defer e.Close()
272-
TestTransactionScriptWithEngine(t, e, harness, script)
272+
TestTransactionScriptWithEngine(t, e, harness, script, false)
273273
})
274274
}
275275

276-
// TestTransactionScriptWithEngine runs the transaction test script given with the engine provided.
277-
func TestTransactionScriptWithEngine(t *testing.T, e QueryEngine, harness Harness, script queries.TransactionTest) {
276+
// TestTransactionScriptPrepared runs the test script given using prepared statements.
277+
func TestTransactionScriptPrepared(t *testing.T, harness Harness, script queries.TransactionTest) bool {
278+
return t.Run(script.Name, func(t *testing.T) {
279+
harness.Setup(setup.MydbData)
280+
e := mustNewEngine(t, harness)
281+
defer e.Close()
282+
TestTransactionScriptWithEngine(t, e, harness, script, true)
283+
})
284+
}
285+
286+
// TestTransactionScriptWithEngine runs the transaction test script given with the engine provided. If |prepared| is true
287+
// then the queries will be prepared and then executed, otherwise they will be executed directly.
288+
func TestTransactionScriptWithEngine(t *testing.T, e QueryEngine, harness Harness, script queries.TransactionTest, prepared bool) {
278289
setupSession := NewSession(harness)
279290
for _, statement := range script.SetUpScript {
280291
if sh, ok := harness.(SkippingHarness); ok {
@@ -307,17 +318,40 @@ func TestTransactionScriptWithEngine(t *testing.T, e QueryEngine, harness Harnes
307318
}
308319

309320
if assertion.ExpectedErr != nil {
310-
AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, assertion.ExpectedErr)
321+
if prepared {
322+
AssertErrPreparedWithCtx(t, e, harness, clientSession, assertion.Query, assertion.ExpectedErr)
323+
} else {
324+
AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, assertion.ExpectedErr)
325+
}
311326
} else if assertion.ExpectedErrStr != "" {
312-
AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, nil, assertion.ExpectedErrStr)
327+
if prepared {
328+
AssertErrPreparedWithCtx(t, e, harness, clientSession, assertion.Query, nil, assertion.ExpectedErrStr)
329+
} else {
330+
AssertErrWithCtx(t, e, harness, clientSession, assertion.Query, assertion.Bindings, nil, assertion.ExpectedErrStr)
331+
}
313332
} else if assertion.ExpectedWarning != 0 {
314-
AssertWarningAndTestQuery(t, e, nil, harness, assertion.Query, assertion.Expected,
315-
nil, assertion.ExpectedWarning, assertion.ExpectedWarningsCount,
316-
assertion.ExpectedWarningMessageSubstring, false)
333+
if prepared {
334+
// TODO: Looks like we don't have a prepared version of this yet
335+
AssertWarningAndTestQuery(t, e, nil, harness, assertion.Query, assertion.Expected,
336+
nil, assertion.ExpectedWarning, assertion.ExpectedWarningsCount,
337+
assertion.ExpectedWarningMessageSubstring, false)
338+
} else {
339+
AssertWarningAndTestQuery(t, e, nil, harness, assertion.Query, assertion.Expected,
340+
nil, assertion.ExpectedWarning, assertion.ExpectedWarningsCount,
341+
assertion.ExpectedWarningMessageSubstring, false)
342+
}
317343
} else if assertion.SkipResultsCheck {
318-
RunQueryWithContext(t, e, harness, clientSession, assertion.Query)
344+
if prepared {
345+
runQueryPreparedWithCtx(t, clientSession, e, assertion.Query, assertion.Bindings, false)
346+
} else {
347+
RunQueryWithContext(t, e, harness, clientSession, assertion.Query)
348+
}
319349
} else {
320-
TestQueryWithContext(t, clientSession, e, harness, assertion.Query, assertion.Expected, nil, nil, nil)
350+
if prepared {
351+
TestPreparedQueryWithContext(t, clientSession, e, harness, assertion.Query, assertion.Expected, nil, nil, false)
352+
} else {
353+
TestQueryWithContext(t, clientSession, e, harness, assertion.Query, assertion.Expected, nil, nil, nil)
354+
}
321355
}
322356
})
323357
}
@@ -551,7 +585,7 @@ func injectBindVarsAndPrepare(
551585

552586
switch p := parsed.(type) {
553587
case *sqlparser.Load, *sqlparser.Prepare, *sqlparser.Execute:
554-
// LOAD DATA query cannot be used as PREPARED STATEMENT
588+
// LOAD DATA, PREPARE, and EXECUTE queries cannot be used as prepared statements
555589
return q, nil, nil
556590
case *sqlparser.Set:
557591
// SET system variable query cannot be used as PREPARED STATEMENT
@@ -564,7 +598,7 @@ func injectBindVarsAndPrepare(
564598

565599
b := planbuilder.New(ctx, e.EngineAnalyzer().Catalog, e.EngineEventScheduler(), nil)
566600
b.SetParserOptions(sql.LoadSqlMode(ctx).ParserOptions())
567-
resPlan, _, err := b.BindOnly(parsed, q, nil)
601+
resPlan, err := e.PrepareQuery(ctx, q)
568602
if err != nil {
569603
return q, nil, err
570604
}

enginetest/queries/transaction_queries.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1447,4 +1447,35 @@ var TransactionTests = []TransactionTest{
14471447
},
14481448
},
14491449
},
1450+
{
1451+
// Repro for https://github.com/dolthub/dolt/issues/3402
1452+
// and https://github.com/dolthub/dolt/issues/9213 (similar issue, just with prepared statements)
1453+
Name: "DDL changes from transactions are available before analyzing statements in other sessions (autocommit on)",
1454+
Assertions: []ScriptTestAssertion{
1455+
{
1456+
Query: "/* client a */ select @@autocommit;",
1457+
Expected: []sql.Row{{1}},
1458+
},
1459+
{
1460+
Query: "/* client b */ select @@autocommit;",
1461+
Expected: []sql.Row{{1}},
1462+
},
1463+
{
1464+
Query: "/* client a */ show tables like 't';",
1465+
Expected: []sql.Row{},
1466+
},
1467+
{
1468+
Query: "/* client b */ show tables like 't';",
1469+
Expected: []sql.Row{},
1470+
},
1471+
{
1472+
Query: "/* client a */ create table t(pk int primary key);",
1473+
Expected: []sql.Row{{types.OkResult{}}},
1474+
},
1475+
{
1476+
Query: "/* client b */ select count(*) from t;",
1477+
Expected: []sql.Row{{0}},
1478+
},
1479+
},
1480+
},
14501481
}

0 commit comments

Comments
 (0)