From f3df8ac041979971671e485db89dc38345c7c3a7 Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 24 Apr 2025 12:13:49 -0700 Subject: [PATCH 1/2] no implicit commits for temporary tables --- enginetest/queries/transaction_queries.go | 80 +++++++++++++++++++++++ sql/planbuilder/builder.go | 1 - sql/planbuilder/ddl.go | 9 +++ 3 files changed, 89 insertions(+), 1 deletion(-) diff --git a/enginetest/queries/transaction_queries.go b/enginetest/queries/transaction_queries.go index 6c39d6994f..205afc776e 100644 --- a/enginetest/queries/transaction_queries.go +++ b/enginetest/queries/transaction_queries.go @@ -1117,6 +1117,86 @@ var TransactionTests = []TransactionTest{ }, }, }, + { + Name: "create temporary table queries are not implicitly committed", + Assertions: []ScriptTestAssertion{ + { + Query: "/* client a */ create table t (pk int primary key);", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{}, + }, + + { + Query: "/* client a */ set @@autocommit = 0;", + Expected: []sql.Row{{}}, + }, + { + Query: "/* client a */ start transaction;", + Expected: []sql.Row{}, + }, + { + // This should not appear for client b until a transaction is committed + Query: "/* client a */ insert into t values (1), (2), (3);", + Expected: []sql.Row{{types.OkResult{RowsAffected: 3}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{}, + }, + + { + // This should not implicitly commit the transaction + Query: "/* client a */ create temporary table tmp1 (pk int primary key);", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + // This should not implicitly commit the transaction + Query: "/* client a */ create temporary table tmp2 (pk int primary key);", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{}, + }, + + { + // This should not implicitly commit the transaction + Query: "/* client a */ insert into tmp1 values (1), (2), (3);", + Expected: []sql.Row{{types.OkResult{RowsAffected: 3}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{}, + }, + + { + // This should not implicitly commit the transaction + Query: "/* client a */ drop temporary table tmp1;", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{}, + }, + + { + // Oddly, this does commit the transaction + Query: "/* client a */ drop table tmp2;", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{ + {1}, + {2}, + {3}, + }, + }, + }, + }, { Name: "alter table queries are implicitly committed", Assertions: []ScriptTestAssertion{ diff --git a/sql/planbuilder/builder.go b/sql/planbuilder/builder.go index 6ca527e1ce..740b6bc3ec 100644 --- a/sql/planbuilder/builder.go +++ b/sql/planbuilder/builder.go @@ -233,7 +233,6 @@ func (b *Builder) buildSubquery(inScope *scope, stmt ast.Statement, subQuery str case *ast.Show: return b.buildShow(inScope, n) case *ast.DDL: - b.qFlags.Set(sql.QFlagDDL) return b.buildDDL(inScope, subQuery, fullQuery, n) case *ast.AlterTable: b.qFlags.Set(sql.QFlagAlterTable) diff --git a/sql/planbuilder/ddl.go b/sql/planbuilder/ddl.go index 588bff1214..b424a751a4 100644 --- a/sql/planbuilder/ddl.go +++ b/sql/planbuilder/ddl.go @@ -122,6 +122,9 @@ func (b *Builder) buildDDL(inScope *scope, subQuery string, fullQuery string, c if err := b.cat.AuthorizationHandler().HandleAuth(b.ctx, b.authQueryState, c.Auth); err != nil && b.authEnabled { b.handleErr(err) } + if !c.Temporary { + b.qFlags.Set(sql.QFlagDDL) + } outScope = inScope.push() switch strings.ToLower(c.Action) { @@ -231,6 +234,7 @@ func (b *Builder) buildDropTable(inScope *scope, c *ast.DDL) (outScope *scope) { if dbName == "" { dbName = b.currentDb().Name() } + for _, t := range c.FromTables { if t.DbQualifier.String() != "" && t.DbQualifier.String() != dbName { err := sql.ErrUnsupportedFeature.New("dropping tables on multiple databases in the same statement") @@ -360,6 +364,11 @@ func (b *Builder) buildCreateTable(inScope *scope, c *ast.DDL) (outScope *scope) database, c.Table.Name.String(), c.IfNotExists, c.Temporary, tableSpec) } + // Temporary tables do not cause implicit commits + if c.Temporary { + b.qFlags.Unset(sql.QFlagDDL) + } + return } From 63df90decbd05c0b1f5f0c79a775cd27a39f2a16 Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 24 Apr 2025 12:28:21 -0700 Subject: [PATCH 2/2] no implicit commit for ddl on temporary tables --- enginetest/queries/transaction_queries.go | 61 +++++++++++++++++++---- sql/planbuilder/ddl.go | 5 -- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/enginetest/queries/transaction_queries.go b/enginetest/queries/transaction_queries.go index 205afc776e..77a68e381e 100644 --- a/enginetest/queries/transaction_queries.go +++ b/enginetest/queries/transaction_queries.go @@ -1118,7 +1118,7 @@ var TransactionTests = []TransactionTest{ }, }, { - Name: "create temporary table queries are not implicitly committed", + Name: "certain ddl queries on temporary tables are not implicitly committed", Assertions: []ScriptTestAssertion{ { Query: "/* client a */ create table t (pk int primary key);", @@ -1149,13 +1149,18 @@ var TransactionTests = []TransactionTest{ { // This should not implicitly commit the transaction - Query: "/* client a */ create temporary table tmp1 (pk int primary key);", + Query: "/* client a */ create temporary table tmp (pk int primary key);", Expected: []sql.Row{{types.OkResult{}}}, }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{}, + }, + { // This should not implicitly commit the transaction - Query: "/* client a */ create temporary table tmp2 (pk int primary key);", - Expected: []sql.Row{{types.OkResult{}}}, + Query: "/* client a */ insert into tmp values (1), (2), (3);", + Expected: []sql.Row{{types.OkResult{RowsAffected: 3}}}, }, { Query: "/* client b */ select * from t;", @@ -1164,8 +1169,8 @@ var TransactionTests = []TransactionTest{ { // This should not implicitly commit the transaction - Query: "/* client a */ insert into tmp1 values (1), (2), (3);", - Expected: []sql.Row{{types.OkResult{RowsAffected: 3}}}, + Query: "/* client a */ drop temporary table tmp;", + Expected: []sql.Row{{types.OkResult{}}}, }, { Query: "/* client b */ select * from t;", @@ -1174,21 +1179,57 @@ var TransactionTests = []TransactionTest{ { // This should not implicitly commit the transaction - Query: "/* client a */ drop temporary table tmp1;", + Query: "/* client a */ create temporary table tmp (pk int primary key);", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + // Oddly, this does implicitly commit the transaction + Query: "/* client a */ drop table tmp;", + Expected: []sql.Row{{types.OkResult{}}}, + }, + { + Query: "/* client b */ select * from t;", + Expected: []sql.Row{ + {1}, + {2}, + {3}, + }, + }, + { + Query: "/* client a */ delete from t where true;", + Expected: []sql.Row{{types.OkResult{RowsAffected: 3}}}, + }, + + { + // This should commit and reset table t + Query: "/* client a */ start transaction;", + Expected: []sql.Row{}, + }, + { + // This should not implicitly commit the transaction + Query: "/* client a */ insert into t values (1), (2), (3);", + Expected: []sql.Row{{types.OkResult{RowsAffected: 3}}}, + }, + { + // This should not implicitly commit the transaction + Query: "/* client a */ create temporary table tmp (pk int primary key);", Expected: []sql.Row{{types.OkResult{}}}, }, { Query: "/* client b */ select * from t;", Expected: []sql.Row{}, }, - { - // Oddly, this does commit the transaction - Query: "/* client a */ drop table tmp2;", + // TODO: turns out we can't alter temporary tables; unskip tests when that is fixed + // Oddly, this does implicitly commit the transaction + Skip: true, + Query: "/* client a */ alter table tmp add column j int;", Expected: []sql.Row{{types.OkResult{}}}, }, { + // TODO: turns out we can't alter temporary tables; unskip tests when that is fixed Query: "/* client b */ select * from t;", + Skip: true, Expected: []sql.Row{ {1}, {2}, diff --git a/sql/planbuilder/ddl.go b/sql/planbuilder/ddl.go index b424a751a4..ca09797bcb 100644 --- a/sql/planbuilder/ddl.go +++ b/sql/planbuilder/ddl.go @@ -364,11 +364,6 @@ func (b *Builder) buildCreateTable(inScope *scope, c *ast.DDL) (outScope *scope) database, c.Table.Name.String(), c.IfNotExists, c.Temporary, tableSpec) } - // Temporary tables do not cause implicit commits - if c.Temporary { - b.qFlags.Unset(sql.QFlagDDL) - } - return }