ccl: regionless restores with regional by row table(s) fails fast

Previously, backing up an MR cluster/db/table that had a regional by row table did not work out-of-the-box (users could not write to said table without performing some operations to ensure hidden column added by regional by row enablement, `crdb_region`, and zone config behaved according to the target RESTORE cluster. This was inadequate because we allowed users to perform a RESTORE where some tables were not "available" right away. This patch addresses this by making sure we "fail fast" if users do decide to attempt a RESTORE where the BACKUP object has a regional by row table. Also: tests. A small amount of tests were fixed to separate the behavior of restoring a database vs a table (just dropping a table after we performed a db restore to test table restore is not a properly isolated test of behavior). Epic: none Informs: cockroachdb#111348 Part of: https://cockroachlabs.atlassian.net/browse/CRDB-29129 Release note (sql change): Users are not able to perform a `remove_regions` RESTORE if the object being restored contains a regional by row table.
annrpom · Oct 5, 2023 · 2dbc10c · 2dbc10c
1 parent 0bf7dd5
commit 2dbc10c
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 187 deletions.
diff --git a/pkg/ccl/backupccl/restore_multiregion_rbr_test.go b/pkg/ccl/backupccl/restore_multiregion_rbr_test.go
@@ -27,11 +27,9 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// The goal of this test is to ensure that if a user ever performed a
-// regionless restore where the backed-up target has a regional by row table,
-// they would be able to get themselves out of a stuck state without needing
-// an enterprise license (in addition to testing the ability to use remove_regions
-// without said license).
+// The goal of this test is to ensure that a user is able to perform a
+// regionless restore and modify the restored object without an enterprise
+// license.
 func TestMultiRegionRegionlessRestoreNoLicense(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -56,9 +54,6 @@ func TestMultiRegionRegionlessRestoreNoLicense(t *testing.T) {
             INSERT INTO d.t VALUES (1), (2), (3);`,
 	)
 
-	// Make table regional by row.
-	mrSql.Exec(t, `ALTER TABLE d.t SET LOCALITY REGIONAL BY ROW;`)
-
 	if err := backuptestutils.VerifyBackupRestoreStatementResult(t, mrSql, `BACKUP DATABASE d INTO $1`, localFoo); err != nil {
 		t.Fatal(err)
 	}
@@ -85,12 +80,6 @@ func TestMultiRegionRegionlessRestoreNoLicense(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Get us in the state that allows us to perform writes.
-	// This is the main purpose of this test - we want to ensure that this process is available
-	// to those without enterprise licenses.
-	sqlDB.Exec(t, `ALTER TABLE d.t ALTER COLUMN crdb_region SET DEFAULT 'us-east1';
-                        ALTER TABLE d.t CONFIGURE ZONE DISCARD;`)
-
 	// Perform some writes to d's table.
 	sqlDB.Exec(t, `INSERT INTO d.t VALUES (4), (5), (6)`)
 

diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go
@@ -1880,6 +1880,14 @@ func doRestorePlan(
 		}
 	}
 
+	if restoreStmt.Options.RemoveRegions {
+		for _, t := range tablesByID {
+			if t.LocalityConfig.GetRegionalByRow() != nil {
+				return errors.Newf("cannot perform a remove_regions RESTORE with region by row enabled table %s in BACKUP target", t.Name)
+			}
+		}
+	}
+
 	if !restoreStmt.Options.SkipLocalitiesCheck {
 		if err := checkClusterRegions(ctx, p, typesByID); err != nil {
 			return err

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-alter-region b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-alter-region
@@ -274,7 +274,11 @@ subtest end
 subtest restore_regionless_on_single_region_table
 
 exec-sql
-DROP TABLE d.t;
+DROP DATABASE d;
+----
+
+exec-sql
+CREATE DATABASE d;
 ----
 
 exec-sql
@@ -284,7 +288,7 @@ RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/table_backup/' WITH remove_regio
 query-sql
 SELECT table_name, locality FROM [SHOW TABLES FROM d] ORDER BY 1;
 ----
-t REGIONAL BY TABLE IN PRIMARY REGION
+t <nil>
 
 # ensure that tables belonging to d can be modified & have the correct values
 exec-sql
@@ -307,7 +311,8 @@ subtest restore_regionless_on_single_region_table/alter-region
 exec-sql
 ALTER DATABASE d ADD REGION "us-east-1";
 ----
-pq: region "us-east-1" already added to database
+pq: cannot add region "us-east-1" to database d
+HINT: you must add a PRIMARY REGION first using ALTER DATABASE d PRIMARY REGION "us-east-1"
 
 exec-sql
 ALTER DATABASE d PRIMARY REGION "us-east-1";

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-on-regionless b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-on-regionless
@@ -152,7 +152,11 @@ subtest end
 subtest restore_regionless_on_regionless_table
 
 exec-sql
-DROP TABLE d.t;
+DROP DATABASE d;
+----
+
+exec-sql
+CREATE DATABASE d;
 ----
 
 exec-sql

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-regional-by-row b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-regional-by-row
@@ -55,186 +55,22 @@ BACKUP INTO 'nodelocal://1/rbr_cluster_backup/';
 
 subtest end
 
+# ensure restoring a cluster/db/table with a table that has regional by row on a single region cluster
+# fails fast
 new-cluster name=s2 share-io-dir=s1 allow-implicit-access localities=us-east-1
 ----
 
-# restoring a cluster with a table that has regional by row on a single region cluster
-subtest restore_regionless_cluster_rbr
-
 exec-sql
 RESTORE FROM LATEST IN 'nodelocal://1/rbr_cluster_backup/' WITH remove_regions;
 ----
-
-# check cluster's regions
-query-sql
-SHOW REGIONS FROM CLUSTER;
-----
-us-east-1 {us-east1}
-
-query-sql
-SHOW DATABASES;
-----
-d root <nil> <nil> {} <nil>
-data root <nil> <nil> {} <nil>
-defaultdb root <nil> <nil> {} <nil>
-postgres root <nil> <nil> {} <nil>
-system node <nil> <nil> {} <nil>
-
-# ensure that database d is regionless
-query-sql
-SELECT region FROM [SHOW REGIONS FROM DATABASE d] ORDER BY 1;
-----
-
-# show tables - make sure these are regionless as well
-query-sql
-SELECT table_name, locality FROM [SHOW TABLES FROM d] ORDER BY 1;
-----
-t <nil>
-
-# ensure that tables belonging to d can be modified & have the correct values
-exec-sql
-INSERT INTO d.t VALUES (4), (5);
-----
-pq: default_to_database_primary_region(): current database defaultdb is not multi-region enabled
-
-# here is how we unblock ourselves from the error above
-exec-sql
-ALTER TABLE d.t ALTER COLUMN crdb_region SET DEFAULT 'us-east-1';
-----
-
-exec-sql
-ALTER TABLE d.t CONFIGURE ZONE DISCARD;
-----
-
-exec-sql
-INSERT INTO d.t VALUES (4), (5);
-----
-
-query-sql
-SELECT * FROM d.t;
-----
-1
-2
-3
-4
-5
-
-subtest end
-
-# restoring a database with a table that has regional by row on a single region cluster
-subtest restore_regionless_rbr_db
-
-exec-sql
-DROP DATABASE d;
-----
+pq: cannot perform a remove_regions RESTORE with region by row enabled table t in BACKUP target
 
 exec-sql
 RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/rbr_database_backup/' WITH remove_regions;
 ----
-
-# check to see if restored database, d, shows up
-query-sql
-SHOW DATABASES;
-----
-d root <nil> <nil> {} <nil>
-data root <nil> <nil> {} <nil>
-defaultdb root <nil> <nil> {} <nil>
-postgres root <nil> <nil> {} <nil>
-system node <nil> <nil> {} <nil>
-
-# ensure that database d is regionless
-query-sql
-SELECT region FROM [SHOW REGIONS FROM DATABASE d] ORDER BY 1;
-----
-
-# show tables - make sure these are regionless as well
-query-sql
-SELECT table_name, locality FROM [SHOW TABLES FROM d] ORDER BY 1;
-----
-t <nil>
-
-# ensure that tables belonging to d can be modified & have the correct values
-exec-sql
-INSERT INTO d.t VALUES (4), (5);
-----
-pq: default_to_database_primary_region(): current database defaultdb is not multi-region enabled
-
-# here is how we unblock ourselves from the error above
-exec-sql
-ALTER TABLE d.t ALTER COLUMN crdb_region SET DEFAULT 'us-east-1';
-----
-
-exec-sql
-ALTER TABLE d.t CONFIGURE ZONE DISCARD;
-----
-
-exec-sql
-INSERT INTO d.t VALUES (4), (5);
-----
-
-query-sql
-SELECT * FROM d.t;
-----
-1
-2
-3
-4
-5
-
-subtest end
-
-# restoring a table that has regional by row on a single region cluster
-subtest restore_regionless_rbr_table
-
-exec-sql
-DROP TABLE d.t;
-----
+pq: cannot perform a remove_regions RESTORE with region by row enabled table t in BACKUP target
 
 exec-sql
 RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/rbr_table_backup/' WITH remove_regions;
 ----
-pq: "crdb_internal_region" is not compatible with type "crdb_internal_region" existing in cluster: "crdb_internal_region" of type "ENUM" is not compatible with type "MULTIREGION_ENUM"
-
-# let's drop the type
-exec-sql
-DROP TYPE d.public.crdb_internal_region;
-----
-
-exec-sql
-RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/rbr_table_backup/' WITH remove_regions;
-----
-
-query-sql
-SELECT table_name, locality FROM [SHOW TABLES FROM d] ORDER BY 1;
-----
-t <nil>
-
-# ensure that tables belonging to d can be modified & have the correct values
-exec-sql
-INSERT INTO d.t VALUES (4), (5);
-----
-pq: default_to_database_primary_region(): current database defaultdb is not multi-region enabled
-
-# here is how we unblock ourselves from the error above
-exec-sql
-ALTER TABLE d.t ALTER COLUMN crdb_region SET DEFAULT 'us-east-1';
-----
-
-exec-sql
-ALTER TABLE d.t CONFIGURE ZONE DISCARD;
-----
-
-exec-sql
-INSERT INTO d.t VALUES (4), (5);
-----
-
-query-sql
-SELECT * FROM d.t;
-----
-1
-2
-3
-4
-5
-
-subtest end
+pq: cannot perform a remove_regions RESTORE with region by row enabled table t in BACKUP target