From 101b56905d5bb4a702d66b506384ceea6467d77f Mon Sep 17 00:00:00 2001 From: Annie Pompa Date: Wed, 27 Sep 2023 12:19:42 -0400 Subject: [PATCH] ccl: syntax change restore option strip_localities to remove_regions This changes the syntax from newly added RESTORE option, strip_localities, to remove_regions. The former was inadequate because it was too similar to an existing option, along with not being an accurately descriptive enough name. Epic: none Informs: #111348 Part of: https://cockroachlabs.atlassian.net/browse/CRDB-29129 Release note (sql change): the syntax for RESTORE option, strip_localities, is now remove_regions. --- docs/generated/sql/bnf/restore_options.bnf | 2 +- docs/generated/sql/bnf/stmt_block.bnf | 6 +++--- pkg/ccl/backupccl/restore_job.go | 4 ++-- .../backupccl/restore_multiregion_rbr_test.go | 4 ++-- pkg/ccl/backupccl/restore_planning.go | 6 +++--- .../restore-regionless-alter-region | 12 +++++------ .../restore-regionless-on-regionless | 12 +++++------ .../restore-regionless-regional-by-row | 8 ++++---- pkg/jobs/jobspb/jobs.proto | 4 ++-- pkg/sql/parser/sql.y | 12 +++++------ pkg/sql/parser/testdata/backup_restore | 10 +++++----- pkg/sql/sem/tree/backup.go | 20 +++++++++---------- 12 files changed, 50 insertions(+), 50 deletions(-) diff --git a/docs/generated/sql/bnf/restore_options.bnf b/docs/generated/sql/bnf/restore_options.bnf index 91ea999e9c54..5d6c7dcba2ea 100644 --- a/docs/generated/sql/bnf/restore_options.bnf +++ b/docs/generated/sql/bnf/restore_options.bnf @@ -21,4 +21,4 @@ restore_options ::= | 'UNSAFE_RESTORE_INCOMPATIBLE_VERSION' | 'EXECUTION' 'LOCALITY' '=' string_or_placeholder | 'EXPERIMENTAL' 'DEFERRED' 'COPY' - | 'STRIP_LOCALITIES' + | 'REMOVE_REGIONS' diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index acbee4c11ce3..9eca79ac53c7 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -1332,6 +1332,7 @@ unreserved_keyword ::= | 'RELATIVE' | 'RELEASE' | 'RELOCATE' + | 'REMOVE_REGIONS' | 'RENAME' | 'REPEATABLE' | 'REPLACE' @@ -1394,7 +1395,6 @@ unreserved_keyword ::= | 'SKIP_MISSING_SEQUENCE_OWNERS' | 'SKIP_MISSING_VIEWS' | 'SKIP_MISSING_UDFS' - | 'STRIP_LOCALITIES' | 'SNAPSHOT' | 'SPLIT' | 'SQL' @@ -2675,7 +2675,7 @@ restore_options ::= | 'UNSAFE_RESTORE_INCOMPATIBLE_VERSION' | 'EXECUTION' 'LOCALITY' '=' string_or_placeholder | 'EXPERIMENTAL' 'DEFERRED' 'COPY' - | 'STRIP_LOCALITIES' + | 'REMOVE_REGIONS' scrub_option_list ::= ( scrub_option ) ( ( ',' scrub_option ) )* @@ -3918,6 +3918,7 @@ bare_label_keywords ::= | 'RELATIVE' | 'RELEASE' | 'RELOCATE' + | 'REMOVE_REGIONS' | 'RENAME' | 'REPEATABLE' | 'REPLACE' @@ -3984,7 +3985,6 @@ bare_label_keywords ::= | 'SKIP_MISSING_SEQUENCE_OWNERS' | 'SKIP_MISSING_UDFS' | 'SKIP_MISSING_VIEWS' - | 'STRIP_LOCALITIES' | 'SMALLINT' | 'SNAPSHOT' | 'SOME' diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index 360c78486940..8405f6a596ca 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -1053,7 +1053,7 @@ func createImportingDescriptors( typesByID[types[i].GetID()] = types[i] } - if details.StripLocalities { + if details.RemoveRegions { // Can't restore multi-region tables into non-multi-region database for _, t := range tables { t.TableDesc().LocalityConfig = nil @@ -1093,7 +1093,7 @@ func createImportingDescriptors( // we need to make sure that multi-region databases no longer get tagged as such - meaning // that we want to change the TypeDescriptor_MULTIREGION_ENUM to a normal enum. We `continue` // to skip the multi-region work below. - if details.StripLocalities { + if details.RemoveRegions { t.TypeDesc().Kind = descpb.TypeDescriptor_ENUM t.TypeDesc().RegionConfig = nil continue diff --git a/pkg/ccl/backupccl/restore_multiregion_rbr_test.go b/pkg/ccl/backupccl/restore_multiregion_rbr_test.go index a5f67cb44185..a3df50a47e70 100644 --- a/pkg/ccl/backupccl/restore_multiregion_rbr_test.go +++ b/pkg/ccl/backupccl/restore_multiregion_rbr_test.go @@ -30,7 +30,7 @@ import ( // 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 strip_localities +// an enterprise license (in addition to testing the ability to use remove_regions // without said license). func TestMultiRegionRegionlessRestoreNoLicense(t *testing.T) { defer leaktest.AfterTest(t)() @@ -81,7 +81,7 @@ func TestMultiRegionRegionlessRestoreNoLicense(t *testing.T) { defer sqlTC.Stopper().Stop(ctx) sqlDB := sqlutils.MakeSQLRunner(sqlTC.Conns[0]) - if err := backuptestutils.VerifyBackupRestoreStatementResult(t, sqlDB, `RESTORE DATABASE d FROM LATEST IN $1 WITH strip_localities`, localFoo); err != nil { + if err := backuptestutils.VerifyBackupRestoreStatementResult(t, sqlDB, `RESTORE DATABASE d FROM LATEST IN $1 WITH remove_regions`, localFoo); err != nil { t.Fatal(err) } diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go index 7fea786052ce..95b58edf0634 100644 --- a/pkg/ccl/backupccl/restore_planning.go +++ b/pkg/ccl/backupccl/restore_planning.go @@ -1081,7 +1081,7 @@ func resolveOptionsForRestoreJobDescription( VerifyData: opts.VerifyData, UnsafeRestoreIncompatibleVersion: opts.UnsafeRestoreIncompatibleVersion, ExperimentalOnline: opts.ExperimentalOnline, - StripLocalities: opts.StripLocalities, + RemoveRegions: opts.RemoveRegions, } if opts.EncryptionPassphrase != nil { @@ -2075,7 +2075,7 @@ func doRestorePlan( // If we are stripping localities, wipe tables of their LocalityConfig before we allocate // descriptor rewrites - as validation in remapTables compares these tables with the non-mr // database and fails otherwise - if restoreStmt.Options.StripLocalities { + if restoreStmt.Options.RemoveRegions { for _, t := range filteredTablesByID { t.TableDesc().LocalityConfig = nil } @@ -2197,7 +2197,7 @@ func doRestorePlan( SkipLocalitiesCheck: restoreStmt.Options.SkipLocalitiesCheck, ExecutionLocality: execLocality, ExperimentalOnline: restoreStmt.Options.ExperimentalOnline, - StripLocalities: restoreStmt.Options.StripLocalities, + RemoveRegions: restoreStmt.Options.RemoveRegions, } jr := jobs.Record{ diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-alter-region b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-alter-region index 9c60e9c74085..1fb8ed0c7cc8 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-alter-region +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-alter-region @@ -45,7 +45,7 @@ subtest end new-cluster name=s2 share-io-dir=s1 allow-implicit-access localities=us-east-1 ---- -# test cluster restore with strip_localities +# test cluster restore with remove_regions subtest restore_regionless_on_single_region_cluster query-sql @@ -56,7 +56,7 @@ postgres root {} system node {} exec-sql -RESTORE FROM LATEST IN 'nodelocal://1/cluster_backup/' WITH strip_localities; +RESTORE FROM LATEST IN 'nodelocal://1/cluster_backup/' WITH remove_regions; ---- # check cluster's regions @@ -167,7 +167,7 @@ subtest end subtest end -# test db restore with strip_localities +# test db restore with remove_regions subtest restore_regionless_on_single_region_db exec-sql @@ -175,7 +175,7 @@ DROP DATABASE d; ---- exec-sql -RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/database_backup/' WITH strip_localities; +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/database_backup/' WITH remove_regions; ---- # check to see if restored database, d, shows up @@ -270,7 +270,7 @@ subtest end subtest end -# test table restore with strip_localities +# test table restore with remove_regions subtest restore_regionless_on_single_region_table exec-sql @@ -278,7 +278,7 @@ DROP TABLE d.t; ---- exec-sql -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/table_backup/' WITH strip_localities; +RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/table_backup/' WITH remove_regions; ---- query-sql diff --git a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-on-regionless b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-on-regionless index 4f14da51ca56..df76340f642b 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-on-regionless +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-on-regionless @@ -44,7 +44,7 @@ subtest end new-cluster name=s2 share-io-dir=s1 allow-implicit-access ---- -# test cluster restore with strip_localities +# test cluster restore with remove_regions subtest restore_regionless_on_regionless_cluster query-sql @@ -55,7 +55,7 @@ postgres root {} system node {} exec-sql -RESTORE FROM LATEST IN 'nodelocal://1/cluster_backup/' WITH strip_localities; +RESTORE FROM LATEST IN 'nodelocal://1/cluster_backup/' WITH remove_regions; ---- # check cluster's regions @@ -100,7 +100,7 @@ SELECT * FROM d.t; subtest end -# test db restore with strip_localities +# test db restore with remove_regions subtest restore_regionless_on_regionless_db exec-sql @@ -108,7 +108,7 @@ DROP DATABASE d; ---- exec-sql -RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/database_backup/' WITH strip_localities; +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/database_backup/' WITH remove_regions; ---- # check to see if restored database, d, shows up @@ -148,7 +148,7 @@ SELECT * FROM d.t; subtest end -# test table restore with strip_localities +# test table restore with remove_regions subtest restore_regionless_on_regionless_table exec-sql @@ -156,7 +156,7 @@ DROP TABLE d.t; ---- exec-sql -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/table_backup/' WITH strip_localities; +RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/table_backup/' WITH remove_regions; ---- query-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 index 6fa811de5d4e..e55a5cc34b75 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-regional-by-row +++ b/pkg/ccl/backupccl/testdata/backup-restore/restore-regionless-regional-by-row @@ -62,7 +62,7 @@ new-cluster name=s2 share-io-dir=s1 allow-implicit-access localities=us-east-1 subtest restore_regionless_cluster_rbr exec-sql -RESTORE FROM LATEST IN 'nodelocal://1/rbr_cluster_backup/' WITH strip_localities; +RESTORE FROM LATEST IN 'nodelocal://1/rbr_cluster_backup/' WITH remove_regions; ---- # check cluster's regions @@ -129,7 +129,7 @@ DROP DATABASE d; ---- exec-sql -RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/rbr_database_backup/' WITH strip_localities; +RESTORE DATABASE d FROM LATEST IN 'nodelocal://1/rbr_database_backup/' WITH remove_regions; ---- # check to see if restored database, d, shows up @@ -191,7 +191,7 @@ DROP TABLE d.t; ---- exec-sql -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/rbr_table_backup/' WITH strip_localities; +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" @@ -201,7 +201,7 @@ DROP TYPE d.public.crdb_internal_region; ---- exec-sql -RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/rbr_table_backup/' WITH strip_localities; +RESTORE TABLE d.t FROM LATEST IN 'nodelocal://1/rbr_table_backup/' WITH remove_regions; ---- query-sql diff --git a/pkg/jobs/jobspb/jobs.proto b/pkg/jobs/jobspb/jobs.proto index a9bc42d7acee..a65b996f0b69 100644 --- a/pkg/jobs/jobspb/jobs.proto +++ b/pkg/jobs/jobspb/jobs.proto @@ -489,8 +489,8 @@ message RestoreDetails { // online restore. repeated roachpb.Span download_spans = 32 [(gogoproto.nullable) = false]; - // Strips localities - bool StripLocalities = 33; + // Removes regions. + bool RemoveRegions = 33; // NEXT ID: 34. } diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 0f4a79f2af11..5140c99c5919 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -987,7 +987,7 @@ func (u *sqlSymUnion) beginTransaction() *tree.BeginTransaction { %token RANGE RANGES READ REAL REASON REASSIGN RECURSIVE RECURRING REDACT REF REFERENCES REFRESH %token REGCLASS REGION REGIONAL REGIONS REGNAMESPACE REGPROC REGPROCEDURE REGROLE REGTYPE REINDEX -%token RELATIVE RELOCATE REMOVE_PATH RENAME REPEATABLE REPLACE REPLICATION +%token RELATIVE RELOCATE REMOVE_PATH REMOVE_REGIONS RENAME REPEATABLE REPLACE REPLICATION %token RELEASE RESET RESTART RESTORE RESTRICT RESTRICTED RESUME RETENTION RETURNING RETURN RETURNS RETRY REVISION_HISTORY %token REVOKE RIGHT ROLE ROLES ROLLBACK ROLLUP ROUTINES ROW ROWS RSHIFT RULE RUNNING @@ -995,7 +995,7 @@ func (u *sqlSymUnion) beginTransaction() *tree.BeginTransaction { %token SEARCH SECOND SECONDARY SECURITY SELECT SEQUENCE SEQUENCES %token SERIALIZABLE SERVER SERVICE SESSION SESSIONS SESSION_USER SET SETOF SETS SETTING SETTINGS %token SHARE SHARED SHOW SIMILAR SIMPLE SIZE SKIP SKIP_LOCALITIES_CHECK SKIP_MISSING_FOREIGN_KEYS -%token SKIP_MISSING_SEQUENCES SKIP_MISSING_SEQUENCE_OWNERS SKIP_MISSING_VIEWS SKIP_MISSING_UDFS STRIP_LOCALITIES SMALLINT SMALLSERIAL +%token SKIP_MISSING_SEQUENCES SKIP_MISSING_SEQUENCE_OWNERS SKIP_MISSING_VIEWS SKIP_MISSING_UDFS SMALLINT SMALLSERIAL %token SNAPSHOT SOME SPLIT SQL SQLLOGIN %token STABLE START STATE STATISTICS STATUS STDIN STDOUT STOP STREAM STRICT STRING STORAGE STORE STORED STORING SUBSTRING SUPER %token SUPPORT SURVIVE SURVIVAL SYMMETRIC SYNTAX SYSTEM SQRT SUBSCRIPTION STATEMENTS @@ -3845,9 +3845,9 @@ restore_options: { $$.val = &tree.RestoreOptions{ExperimentalOnline: true} } -| STRIP_LOCALITIES +| REMOVE_REGIONS { - $$.val = &tree.RestoreOptions{StripLocalities: true, SkipLocalitiesCheck: true} + $$.val = &tree.RestoreOptions{RemoveRegions: true, SkipLocalitiesCheck: true} } virtual_cluster_opt: @@ -16891,6 +16891,7 @@ unreserved_keyword: | RELATIVE | RELEASE | RELOCATE +| REMOVE_REGIONS | RENAME | REPEATABLE | REPLACE @@ -16953,7 +16954,6 @@ unreserved_keyword: | SKIP_MISSING_SEQUENCE_OWNERS | SKIP_MISSING_VIEWS | SKIP_MISSING_UDFS -| STRIP_LOCALITIES | SNAPSHOT | SPLIT | SQL @@ -17446,6 +17446,7 @@ bare_label_keywords: | RELATIVE | RELEASE | RELOCATE +| REMOVE_REGIONS | RENAME | REPEATABLE | REPLACE @@ -17512,7 +17513,6 @@ bare_label_keywords: | SKIP_MISSING_SEQUENCE_OWNERS | SKIP_MISSING_UDFS | SKIP_MISSING_VIEWS -| STRIP_LOCALITIES | SMALLINT | SNAPSHOT | SOME diff --git a/pkg/sql/parser/testdata/backup_restore b/pkg/sql/parser/testdata/backup_restore index a397c0b113c9..26cd6b65f7b3 100644 --- a/pkg/sql/parser/testdata/backup_restore +++ b/pkg/sql/parser/testdata/backup_restore @@ -824,12 +824,12 @@ RESTORE TABLE foo FROM '_' WITH OPTIONS (skip_missing_foreign_keys, skip_missing RESTORE TABLE _ FROM 'bar' WITH OPTIONS (skip_missing_foreign_keys, skip_missing_sequences, detached) -- identifiers removed parse -RESTORE TABLE foo FROM 'bar' WITH strip_localities +RESTORE TABLE foo FROM 'bar' WITH remove_regions ---- -RESTORE TABLE foo FROM 'bar' WITH OPTIONS (skip_localities_check, strip_localities) -- normalized! -RESTORE TABLE (foo) FROM ('bar') WITH OPTIONS (skip_localities_check, strip_localities) -- fully parenthesized -RESTORE TABLE foo FROM '_' WITH OPTIONS (skip_localities_check, strip_localities) -- literals removed -RESTORE TABLE _ FROM 'bar' WITH OPTIONS (skip_localities_check, strip_localities) -- identifiers removed +RESTORE TABLE foo FROM 'bar' WITH OPTIONS (skip_localities_check, remove_regions) -- normalized! +RESTORE TABLE (foo) FROM ('bar') WITH OPTIONS (skip_localities_check, remove_regions) -- fully parenthesized +RESTORE TABLE foo FROM '_' WITH OPTIONS (skip_localities_check, remove_regions) -- literals removed +RESTORE TABLE _ FROM 'bar' WITH OPTIONS (skip_localities_check, remove_regions) -- identifiers removed parse BACKUP INTO 'bar' WITH include_all_virtual_clusters = $1, detached diff --git a/pkg/sql/sem/tree/backup.go b/pkg/sql/sem/tree/backup.go index e963e09f062c..6b1441d2eaaa 100644 --- a/pkg/sql/sem/tree/backup.go +++ b/pkg/sql/sem/tree/backup.go @@ -149,7 +149,7 @@ type RestoreOptions struct { UnsafeRestoreIncompatibleVersion bool ExecutionLocality Expr ExperimentalOnline bool - StripLocalities bool + RemoveRegions bool } var _ NodeFormatter = &RestoreOptions{} @@ -536,9 +536,9 @@ func (o *RestoreOptions) Format(ctx *FmtCtx) { ctx.WriteString("experimental deferred copy") } - if o.StripLocalities { + if o.RemoveRegions { maybeAddSep() - ctx.WriteString("strip_localities") + ctx.WriteString("remove_regions") } } @@ -612,8 +612,8 @@ func (o *RestoreOptions) CombineWith(other *RestoreOptions) error { } if o.SkipLocalitiesCheck { - // If StripLocalities is true, SkipLocalitiesCheck should also be true - if other.SkipLocalitiesCheck && !other.StripLocalities { + // If RemoveRegions is true, SkipLocalitiesCheck should also be true + if other.SkipLocalitiesCheck && !other.RemoveRegions { return errors.New("skip_localities_check specified multiple times") } } else { @@ -695,12 +695,12 @@ func (o *RestoreOptions) CombineWith(other *RestoreOptions) error { o.ExperimentalOnline = other.ExperimentalOnline } - if o.StripLocalities { - if other.StripLocalities { - return errors.New("strip_localities specified multiple times") + if o.RemoveRegions { + if other.RemoveRegions { + return errors.New("remove_regions specified multiple times") } } else { - o.StripLocalities = other.StripLocalities + o.RemoveRegions = other.RemoveRegions } return nil @@ -730,7 +730,7 @@ func (o RestoreOptions) IsDefault() bool { o.UnsafeRestoreIncompatibleVersion == options.UnsafeRestoreIncompatibleVersion && o.ExecutionLocality == options.ExecutionLocality && o.ExperimentalOnline == options.ExperimentalOnline && - o.StripLocalities == options.StripLocalities + o.RemoveRegions == options.RemoveRegions } // BackupTargetList represents a list of targets.