Skip to content

Commit 71e32a6

Browse files
craig[bot]fqazi
craig[bot]
and
fqazi
committed
Merge #78512
78512: sql: block operations properly on implicit transactions r=fqazi a=fqazi Previously, an enhancement was made to allow implicit transactions to execute multiple statements, which caused code to block operations in implicit transactions to break. This was problematic because these scenarios cared that only a single statement is executed, and not if it was only an implicit transition. For example, the declarative schema changer and legacy schema changer could be mixed in implicit transactions after this change. To address this, this patch modifies places that we're checking for implicit transactions to instead check for single statement transactions. Release note: None Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
2 parents 2ecd90c + 6ba4c1e commit 71e32a6

76 files changed

Lines changed: 366 additions & 116 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎pkg/ccl/backupccl/backup_planning.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -668,7 +668,7 @@ func backupPlanHook(
668668
ctx, span := tracing.ChildSpan(ctx, stmt.StatementTag())
669669
defer span.Finish()
670670

671-
if !(p.IsAutoCommit() || backupStmt.Options.Detached) {
671+
if !(p.ExtendedEvalContext().TxnIsSingleStmt || backupStmt.Options.Detached) {
672672
return errors.Errorf("BACKUP cannot be used inside a multi-statement transaction without DETACHED option")
673673
}
674674

‎pkg/ccl/backupccl/restore_planning.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1149,7 +1149,7 @@ func restorePlanHook(
11491149
ctx, span := tracing.ChildSpan(ctx, stmt.StatementTag())
11501150
defer span.Finish()
11511151

1152-
if !(p.IsAutoCommit() || restoreStmt.Options.Detached) {
1152+
if !(p.ExtendedEvalContext().TxnIsSingleStmt || restoreStmt.Options.Detached) {
11531153
return errors.Errorf("RESTORE cannot be used inside a multi-statement transaction without DETACHED option")
11541154
}
11551155

‎pkg/ccl/backupccl/show_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,14 @@ func TestShowBackup(t *testing.T) {
5050
_, sqlDBRestore, cleanupEmptyCluster := backupRestoreTestSetupEmpty(t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{})
5151
defer cleanupFn()
5252
defer cleanupEmptyCluster()
53-
sqlDB.Exec(t, `
53+
sqlDB.ExecMultiple(t, strings.Split(`
5454
SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE;
5555
CREATE TYPE data.welcome AS ENUM ('hello', 'hi');
5656
USE data; CREATE SCHEMA sc;
5757
CREATE TABLE data.sc.t1 (a INT);
5858
CREATE TABLE data.sc.t2 (a data.welcome);
59-
`)
59+
`,
60+
`;`)...)
6061

6162
const full, inc, inc2 = localFoo + "/full", localFoo + "/inc", localFoo + "/inc2"
6263

@@ -768,13 +769,13 @@ func TestShowBackupWithDebugIDs(t *testing.T) {
768769
defer cleanupFn()
769770

770771
// add 1 type, 1 schema, and 2 tables to the database
771-
sqlDB.Exec(t, `
772+
sqlDB.ExecMultiple(t, strings.Split(`
772773
SET CLUSTER SETTING sql.cross_db_fks.enabled = TRUE;
773774
CREATE TYPE data.welcome AS ENUM ('hello', 'hi');
774775
USE data; CREATE SCHEMA sc;
775776
CREATE TABLE data.sc.t1 (a INT);
776-
CREATE TABLE data.sc.t2 (a data.welcome);
777-
`)
777+
CREATE TABLE data.sc.t2 (a data.welcome);`,
778+
`;`)...)
778779

779780
const full = localFoo + "/full"
780781

‎pkg/ccl/backupccl/testdata/backup-restore/backup-dropped-descriptors‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,13 @@ new-server name=s1
77

88
exec-sql
99
SET use_declarative_schema_changer = 'off';
10+
----
11+
12+
exec-sql
1013
SET CLUSTER SETTING jobs.debug.pausepoints = 'schemachanger.before.exec';
14+
----
15+
16+
exec-sql
1117
CREATE DATABASE d;
1218
CREATE TABLE d.foo (id INT);
1319
----
@@ -118,6 +124,9 @@ CREATE TABLE d2.s.t (id INT);
118124

119125
exec-sql
120126
SET use_declarative_schema_changer = 'off';
127+
----
128+
129+
exec-sql
121130
SET CLUSTER SETTING jobs.debug.pausepoints = 'schemachanger.before.exec';
122131
----
123132

‎pkg/ccl/backupccl/testdata/backup-restore/column-families‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,13 @@ ALTER TABLE cfs SPLIT AT SELECT a FROM cfs;
2424
exec-sql
2525
-- Split the output files very small to catch output SSTs mid-row.
2626
SET CLUSTER SETTING bulkio.backup.file_size = '1';
27+
----
28+
29+
exec-sql
2730
SET CLUSTER SETTING kv.bulk_sst.target_size = '1';
31+
----
32+
33+
exec-sql
2834
SET CLUSTER SETTING bulkio.backup.merge_file_buffer_size = '1MiB';
2935
----
3036

‎pkg/ccl/backupccl/testdata/backup-restore/max-row-size‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ pq: row larger than max row size: table 112 family 0 primary key /Table/112/1/2/
4646

4747
exec-sql
4848
SET CLUSTER SETTING sql.guardrails.max_row_size_err = DEFAULT;
49+
----
50+
51+
exec-sql
4952
INSERT INTO d2.maxrow VALUES (2, repeat('y', 20000));
5053
----
5154

‎pkg/ccl/backupccl/testdata/backup-restore/restore-on-fail-or-cancel-ttl‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ BACKUP DATABASE d INTO 'userfile:///foo'
1818
# Attempt the restore but pause it before publishing descriptors.
1919
exec-sql
2020
SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.before_publishing_descriptors';
21+
----
22+
23+
exec-sql
2124
DROP DATABASE d;
2225
----
2326

@@ -52,7 +55,13 @@ BACKUP DATABASE d INTO 'userfile:///foo'
5255
# Attempt the restore but pause it after publishing descriptors.
5356
exec-sql
5457
SET CLUSTER SETTING jobs.debug.pausepoints = '';
58+
----
59+
60+
exec-sql
5561
SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.after_publishing_descriptors';
62+
----
63+
64+
exec-sql
5665
DROP DATABASE d;
5766
----
5867

‎pkg/ccl/changefeedccl/changefeed_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -374,8 +374,7 @@ func TestChangefeedTenants(t *testing.T) {
374374

375375
tenantServer, tenantDB := serverutils.StartTenant(t, kvServer, tenantArgs)
376376
tenantSQL := sqlutils.MakeSQLRunner(tenantDB)
377-
tenantSQL.Exec(t, serverSetupStatements)
378-
377+
tenantSQL.ExecMultiple(t, strings.Split(serverSetupStatements, ";")...)
379378
tenantSQL.Exec(t, `CREATE TABLE foo_in_tenant (pk INT PRIMARY KEY)`)
380379
t.Run("changefeed on non-tenant table fails", func(t *testing.T) {
381380
kvSQL := sqlutils.MakeSQLRunner(kvSQLdb)
@@ -445,7 +444,7 @@ func TestChangefeedTenantsExternalIOEnabled(t *testing.T) {
445444

446445
tenantServer, tenantDB := serverutils.StartTenant(t, kvServer, tenantArgs)
447446
tenantSQL := sqlutils.MakeSQLRunner(tenantDB)
448-
tenantSQL.Exec(t, serverSetupStatements)
447+
tenantSQL.ExecMultiple(t, strings.Split(serverSetupStatements, ";")...)
449448
tenantSQL.Exec(t, `CREATE TABLE foo_in_tenant (pk INT PRIMARY KEY)`)
450449

451450
t.Run("sinkful changefeed works", func(t *testing.T) {
@@ -952,7 +951,7 @@ func TestChangefeedExternalIODisabled(t *testing.T) {
952951
})
953952
defer s.Stopper().Stop(ctx)
954953
sqlDB := sqlutils.MakeSQLRunner(db)
955-
sqlDB.Exec(t, serverSetupStatements)
954+
sqlDB.ExecMultiple(t, strings.Split(serverSetupStatements, ";")...)
956955
sqlDB.Exec(t, "CREATE TABLE target_table (pk INT PRIMARY KEY)")
957956
for _, proto := range disallowedSinkProtos {
958957
sqlDB.ExpectErr(t, "Outbound IO is disabled by configuration, cannot create changefeed",

‎pkg/ccl/changefeedccl/helpers_test.go‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -359,8 +359,8 @@ func startTestFullServer(
359359
}
360360
}()
361361

362-
_, err = db.ExecContext(ctx, serverSetupStatements)
363-
require.NoError(t, err)
362+
sqlDB := sqlutils.MakeSQLRunner(db)
363+
sqlDB.ExecMultiple(t, strings.Split(serverSetupStatements, ";")...)
364364

365365
if region := serverArgsRegion(args); region != "" {
366366
_, err = db.ExecContext(ctx, fmt.Sprintf(`ALTER DATABASE d PRIMARY REGION "%s"`, region))
@@ -399,8 +399,8 @@ func startTestCluster(t testing.TB) (serverutils.TestClusterInterface, *gosql.DB
399399
require.NoError(t, err)
400400
}
401401
}()
402-
_, err = db.ExecContext(ctx, serverSetupStatements)
403-
require.NoError(t, err)
402+
sqlDB := sqlutils.MakeSQLRunner(db)
403+
sqlDB.ExecMultiple(t, strings.Split(serverSetupStatements, ";")...)
404404

405405
_, err = db.ExecContext(ctx, `ALTER DATABASE d PRIMARY REGION "us-east1"`)
406406
return cluster, db, cleanupAndReset
@@ -409,8 +409,6 @@ func startTestCluster(t testing.TB) (serverutils.TestClusterInterface, *gosql.DB
409409
func startTestTenant(
410410
t testing.TB, options feedTestOptions,
411411
) (serverutils.TestServerInterface, *gosql.DB, func()) {
412-
ctx := context.Background()
413-
414412
kvServer, _, cleanupCluster := startTestFullServer(t, options)
415413
knobs := base.TestingKnobs{
416414
DistSQL: &execinfra.TestingKnobs{Changefeed: &TestingKnobs{}},
@@ -432,8 +430,8 @@ func startTestTenant(
432430

433431
tenantServer, tenantDB := serverutils.StartTenant(t, kvServer, tenantArgs)
434432
// Re-run setup on the tenant as well
435-
_, err := tenantDB.ExecContext(ctx, serverSetupStatements)
436-
require.NoError(t, err)
433+
tenantRunner := sqlutils.MakeSQLRunner(tenantDB)
434+
tenantRunner.ExecMultiple(t, strings.Split(serverSetupStatements, ";")...)
437435

438436
server := &testServerShim{tenantServer, kvServer}
439437
// Log so that it is clear if a failed test happened

‎pkg/ccl/kvccl/kvfollowerreadsccl/testdata/boundedstaleness/single_row‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@ reset-matching-stmt-for-tracing
140140
# Set a super high closed bounded staleness target and execute a schema change.
141141
exec
142142
SET CLUSTER SETTING kv.closed_timestamp.target_duration = '1hr';
143+
----
144+
145+
exec
143146
ALTER TABLE t ADD COLUMN new_col INT NOT NULL DEFAULT 2
144147
----
145148

0 commit comments

Comments
 (0)