Skip to content

Commit

Permalink
sql: execute batch statements in an implicit transaction
Browse files Browse the repository at this point in the history
Release note (bug fix): Previously statements that arrived in a batch
during the simple query protocol would all execute in their own implicit
transactions. Now, we match the Postgres wire protocol, so all these
statements share the same implicit transaction. If a BEGIN is included
in a statement batch, then the existing implicit transaction is
upgraded to an explicit transaction.

Release note (backward-incompatible change): BACKUP and RESTORE
statements can no longer be sent in a batch of other statements.
  • Loading branch information
rafiss committed Feb 28, 2022
1 parent 75f0936 commit 38d4cf7
Show file tree
Hide file tree
Showing 41 changed files with 555 additions and 172 deletions.
4 changes: 2 additions & 2 deletions pkg/ccl/backupccl/backup_planning.go
Original file line number Diff line number Diff line change
Expand Up @@ -652,8 +652,8 @@ func backupPlanHook(
ctx, span := tracing.ChildSpan(ctx, stmt.StatementTag())
defer span.Finish()

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

subdir, err := subdirFn()
Expand Down
41 changes: 18 additions & 23 deletions pkg/ccl/backupccl/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5481,7 +5481,7 @@ func TestDetachedBackup(t *testing.T) {
return tx.QueryRow(`BACKUP DATABASE data TO $1`, localFoo).Scan(&jobID)
})
require.True(t, testutils.IsError(err,
"BACKUP cannot be used inside a transaction without DETACHED option"))
"BACKUP cannot be used inside a multi-statement transaction without DETACHED option"))

// Okay to run DETACHED backup, even w/out explicit transaction.
sqlDB.QueryRow(t, `BACKUP DATABASE data TO $1 WITH DETACHED`, localFoo).Scan(&jobID)
Expand Down Expand Up @@ -5535,7 +5535,7 @@ func TestDetachedRestore(t *testing.T) {
return tx.QueryRow(`RESTORE TABLE t FROM $1 WITH INTO_DB=test`, localFoo).Scan(&jobID)
})
require.True(t, testutils.IsError(err,
"RESTORE cannot be used inside a transaction without DETACHED option"))
"RESTORE cannot be used inside a multi-statement transaction without DETACHED option"))

// Okay to run DETACHED RESTORE, even w/out explicit transaction.
sqlDB.QueryRow(t, `RESTORE TABLE t FROM $1 WITH DETACHED, INTO_DB=test`,
Expand Down Expand Up @@ -9054,11 +9054,9 @@ func TestDroppedDescriptorRevisionAndSystemDBIDClash(t *testing.T) {
_, sqlDB, tempDir, cleanupFn := backupRestoreTestSetup(t, singleNode, 1, InitManualReplication)
defer cleanupFn()

sqlDB.Exec(t, `
CREATE TABLE foo (id INT);
BACKUP TO 'nodelocal://0/foo' WITH revision_history;
DROP TABLE foo;
`)
sqlDB.Exec(t, `CREATE TABLE foo (id INT);`)
sqlDB.Exec(t, `BACKUP TO 'nodelocal://0/foo' WITH revision_history;`)
sqlDB.Exec(t, `DROP TABLE foo;`)

var aost string
sqlDB.QueryRow(t, "SELECT cluster_logical_timestamp()").Scan(&aost)
Expand Down Expand Up @@ -9134,15 +9132,13 @@ func TestRestoreRemappingOfExistingUDTInColExpr(t *testing.T) {
_, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, InitManualReplication)
defer cleanupFn()

sqlDB.Exec(t, `
CREATE TYPE status AS ENUM ('open', 'closed', 'inactive');
CREATE TABLE foo (id INT PRIMARY KEY, what status default 'open');
BACKUP DATABASE data to 'nodelocal://0/foo';
DROP TABLE foo CASCADE;
DROP TYPE status;
CREATE TYPE status AS ENUM ('open', 'closed', 'inactive');
RESTORE TABLE foo FROM 'nodelocal://0/foo';
`)
sqlDB.Exec(t, `CREATE TYPE status AS ENUM ('open', 'closed', 'inactive');`)
sqlDB.Exec(t, `CREATE TABLE foo (id INT PRIMARY KEY, what status default 'open');`)
sqlDB.Exec(t, `BACKUP DATABASE data to 'nodelocal://0/foo';`)
sqlDB.Exec(t, `DROP TABLE foo CASCADE;`)
sqlDB.Exec(t, `DROP TYPE status;`)
sqlDB.Exec(t, `CREATE TYPE status AS ENUM ('open', 'closed', 'inactive');`)
sqlDB.Exec(t, `RESTORE TABLE foo FROM 'nodelocal://0/foo';`)
}

// TestGCDropIndexSpanExpansion is a regression test for
Expand Down Expand Up @@ -9173,13 +9169,12 @@ func TestGCDropIndexSpanExpansion(t *testing.T) {
sqlRunner := sqlutils.MakeSQLRunner(conn)
sqlRunner.Exec(t, `SET CLUSTER SETTING kv.closed_timestamp.target_duration = '100ms'`) // speeds up the test

sqlRunner.Exec(t, `
CREATE DATABASE test; USE test;
CREATE TABLE foo (id INT PRIMARY KEY, id2 INT, id3 INT, INDEX bar (id2), INDEX baz(id3));
ALTER INDEX foo@bar CONFIGURE ZONE USING gc.ttlseconds = '1';
INSERT INTO foo VALUES (1, 2, 3);
DROP INDEX foo@bar;
`)
sqlRunner.Exec(t, `CREATE DATABASE test;`)
sqlRunner.Exec(t, ` USE test;`)
sqlRunner.Exec(t, `CREATE TABLE foo (id INT PRIMARY KEY, id2 INT, id3 INT, INDEX bar (id2), INDEX baz(id3));`)
sqlRunner.Exec(t, `ALTER INDEX foo@bar CONFIGURE ZONE USING gc.ttlseconds = '1';`)
sqlRunner.Exec(t, `INSERT INTO foo VALUES (1, 2, 3);`)
sqlRunner.Exec(t, `DROP INDEX foo@bar;`)

// Wait until the index is about to get gc'ed.
<-aboutToGC
Expand Down
16 changes: 7 additions & 9 deletions pkg/ccl/backupccl/full_cluster_backup_restore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1042,15 +1042,13 @@ func TestClusterRevisionDoesNotBackupOptOutSystemTables(t *testing.T) {
sqlDB := sqlutils.MakeSQLRunner(conn)
defer cleanup()

sqlDB.Exec(t, `
CREATE DATABASE test;
USE test;
CREATE TABLE foo (id INT);
BACKUP TO 'nodelocal://1/foo' WITH revision_history;
BACKUP TO 'nodelocal://1/foo' WITH revision_history;
CREATE TABLE bar (id INT);
BACKUP TO 'nodelocal://1/foo' WITH revision_history;
`)
sqlDB.Exec(t, `CREATE DATABASE test;`)
sqlDB.Exec(t, `USE test;`)
sqlDB.Exec(t, `CREATE TABLE foo (id INT);`)
sqlDB.Exec(t, `BACKUP TO 'nodelocal://1/foo' WITH revision_history;`)
sqlDB.Exec(t, `BACKUP TO 'nodelocal://1/foo' WITH revision_history;`)
sqlDB.Exec(t, `CREATE TABLE bar (id INT);`)
sqlDB.Exec(t, `BACKUP TO 'nodelocal://1/foo' WITH revision_history;`)
}

func TestRestoreWithRecreatedDefaultDB(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/ccl/backupccl/restore_planning.go
Original file line number Diff line number Diff line change
Expand Up @@ -1142,8 +1142,8 @@ func restorePlanHook(
ctx, span := tracing.ChildSpan(ctx, stmt.StatementTag())
defer span.Finish()

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

subdir, err := subdirFn()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ BACKUP INTO 'nodelocal://0/cluster/dropped-database';
# dropped database 'd'.
exec-sql
RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/dropped-database' WITH new_db_name = 'd1';
----

exec-sql
USE d1;
----

Expand All @@ -76,6 +79,9 @@ public bar
# dropped database 'd'.
exec-sql
RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/cluster/dropped-database' WITH new_db_name = 'd2';
----

exec-sql
USE d2;
----

Expand Down Expand Up @@ -158,6 +164,9 @@ WHERE object_name = 's' OR object_name = 'typ';
# Restore the backups to check they are valid.
exec-sql
RESTORE DATABASE d2 FROM LATEST IN 'nodelocal://0/dropped-schema-in-database' WITH new_db_name = 'd3';
----

exec-sql
USE d3;
----

Expand All @@ -180,6 +189,9 @@ public t2

exec-sql
RESTORE DATABASE d2 FROM LATEST IN 'nodelocal://0/cluster/dropped-schema-in-database' WITH new_db_name ='d4';
----

exec-sql
USE d4;
----

Expand Down
16 changes: 16 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/backup-permissions
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ CREATE TABLE d.t (x INT);
INSERT INTO d.t VALUES (1), (2), (3);
----

# BACKUP is not allowed in a batch-statement.
exec-sql
BACKUP TO 'nodelocal://0/test-root/';
SELECT 1;
----
pq: BACKUP cannot be used inside a multi-statement transaction without DETACHED option

# Cluster backup should succeed as a root user.
exec-sql
BACKUP TO 'nodelocal://0/test-root/'
Expand All @@ -22,7 +29,13 @@ GRANT ADMIN TO testuser;

exec-sql user=testuser
BACKUP TO 'nodelocal://0/test-nonroot-cluster';
----

exec-sql user=testuser
BACKUP DATABASE d TO 'nodelocal://0/test-nonroot-db';
----

exec-sql user=testuser
BACKUP TABLE d.t TO 'nodelocal://0/test-nonroot-table';
----

Expand Down Expand Up @@ -106,6 +119,9 @@ GRANT USAGE ON TYPE d2.greeting TO testuser;
# testuser should now have all the required privileges.
exec-sql server=s2 user=testuser
BACKUP DATABASE d2 TO 'nodelocal://0/d2';
----

exec-sql server=s2 user=testuser
BACKUP TABLE d2.t TO 'nodelocal://0/d2-table';
----

Expand Down
12 changes: 12 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/column-families
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ new-server name=s1 localities=us-east-1,us-west-1,us-west-2,eu-central-1

exec-sql
CREATE DATABASE orig;
----

exec-sql
USE orig;
CREATE TABLE cfs (a INT PRIMARY KEY, b STRING, c STRING, d STRING, FAMILY (b), FAMILY (c));
INSERT INTO cfs SELECT x, repeat('abc', 100), repeat('abc', 100) FROM generate_series(0, 3) AS x;
Expand All @@ -11,8 +14,17 @@ ALTER TABLE cfs SPLIT AT SELECT a FROM cfs;
SET CLUSTER SETTING bulkio.backup.file_size = '1';
SET CLUSTER SETTING kv.bulk_sst.target_size = '1';
SET CLUSTER SETTING bulkio.backup.merge_file_buffer_size = '1MiB';
----

exec-sql
BACKUP cfs TO 'nodelocal://1/foo';
----

exec-sql
CREATE DATABASE r1;
----

exec-sql
RESTORE cfs FROM 'nodelocal://1/foo' WITH into_db='r1';
----

Expand Down
12 changes: 12 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/descriptor-broadening
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,26 @@ new-server name=s1
exec-sql
CREATE DATABASE db1;
CREATE DATABASE db2;
----

exec-sql
CREATE TABLE db1.t (a INT);
----

exec-sql
BACKUP DATABASE db1 TO 'nodelocal://1/backup';
----

exec-sql
BACKUP DATABASE db1,db2 TO 'nodelocal://1/backup';
----
pq: previous backup does not contain the complete database "db2"

exec-sql
BACKUP db1.t TO 'nodelocal://1/backup_2';
----

exec-sql
BACKUP DATABASE db1 TO 'nodelocal://1/backup_2';
----
pq: previous backup does not contain the complete database "db1"
12 changes: 12 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/feature-flags
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,19 @@ INSERT INTO d.t VALUES (1), (2), (3);
# Test running backup when BACKUP feature flag is disabled.
exec-sql
SET CLUSTER SETTING feature.backup.enabled = FALSE;
----

exec-sql
BACKUP TO 'nodelocal://0/test-root/';
----
pq: feature BACKUP was disabled by the database administrator

# Test running backup when feature flag is enabled.
exec-sql
SET CLUSTER SETTING feature.backup.enabled = TRUE;
----

exec-sql
BACKUP TO 'nodelocal://0/test-root/';
----

Expand All @@ -27,12 +33,18 @@ DROP TABLE d.t;
# Test running restore when feature flag is disabled.
exec-sql
SET CLUSTER SETTING feature.restore.enabled = FALSE;
----

exec-sql
RESTORE TABLE d.t FROM 'nodelocal://0/test-root/';
----
pq: feature RESTORE was disabled by the database administrator

# Test running restore when feature flag is enabled.
exec-sql
SET CLUSTER SETTING feature.restore.enabled = TRUE;
----

exec-sql
RESTORE TABLE d.t FROM 'nodelocal://0/test-root/';
----
6 changes: 6 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/max-row-size
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ pq: row larger than max row size: table 109 family 0 primary key /Table/109/1/2/

exec-sql
BACKUP maxrow TO 'nodelocal://1/maxrow';
----

exec-sql
CREATE DATABASE d2;
----

exec-sql
RESTORE maxrow FROM 'nodelocal://1/maxrow' WITH into_db='d2';
----

Expand Down
26 changes: 25 additions & 1 deletion pkg/ccl/backupccl/testdata/backup-restore/multiregion
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,34 @@ HINT: there are two ways you can resolve this issue: 1) update the cluster to wh
# Create a database with no regions to check default primary regions.
exec-sql
CREATE DATABASE no_region_db;
----

exec-sql
CREATE TABLE no_region_db.t (x INT);
INSERT INTO no_region_db.t VALUES (1), (2), (3);
CREATE DATABASE no_region_db_2;
CREATE TABLE no_region_db_2.t (x INT);
INSERT INTO no_region_db_2.t VALUES (1), (2), (3);
----

exec-sql
BACKUP DATABASE no_region_db TO 'nodelocal://1/no_region_database_backup/';
----

exec-sql
BACKUP TO 'nodelocal://1/no_region_cluster_backup/';
----

exec-sql
DROP DATABASE no_region_db;
DROP DATABASE no_region_db_2;
----

exec-sql
SET CLUSTER SETTING sql.defaults.primary_region = 'non-existent-region';
----

exec-sql
RESTORE DATABASE no_region_db FROM 'nodelocal://1/no_region_database_backup/';
----
pq: region "non-existent-region" does not exist
Expand All @@ -88,6 +103,9 @@ set the default PRIMARY REGION to a region that exists (see SHOW REGIONS FROM CL

exec-sql
SET CLUSTER SETTING sql.defaults.primary_region = 'eu-central-1';
----

exec-sql
RESTORE DATABASE no_region_db FROM 'nodelocal://1/no_region_database_backup/';
----
NOTICE: setting the PRIMARY REGION as eu-central-1 on database no_region_db
Expand All @@ -111,16 +129,22 @@ exec-sql
CREATE DATABASE eu_central_db;
CREATE TABLE eu_central_db.t (x INT);
INSERT INTO eu_central_db.t VALUES (1), (2), (3);
BACKUP DATABASE eu_central_db TO 'nodelocal://1/eu_central_database_backup/';
----
NOTICE: setting eu-central-1 as the PRIMARY REGION as no PRIMARY REGION was specified

exec-sql
BACKUP DATABASE eu_central_db TO 'nodelocal://1/eu_central_database_backup/';
----

# New cluster for a cluster backup.
new-server name=s4 share-io-dir=s1 allow-implicit-access localities=eu-central-1,eu-north-1
----

exec-sql
SET CLUSTER SETTING sql.defaults.primary_region = 'eu-north-1';
----

exec-sql
RESTORE FROM 'nodelocal://1/no_region_cluster_backup/';
----
NOTICE: setting the PRIMARY REGION as eu-north-1 on database defaultdb
Expand Down
Loading

0 comments on commit 38d4cf7

Please sign in to comment.