release-21.1: backport zone config validation changes

pkg/ccl/backupccl/restore_job.go

@@ -1292,7 +1292,13 @@ func createImportingDescriptors(
 								return err
 							}
 
-							regionConfig, err := sql.SynthesizeRegionConfigOffline(ctx, txn, desc.ID, descsCol)
+							regionConfig, err := sql.SynthesizeRegionConfig(
+								ctx,
+								txn,
+								desc.GetID(),
+								descsCol,
+								sql.SynthesizeRegionConfigOptionIncludeOffline,
+							)
 							if err != nil {
 								return err
 							}

pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs

@@ -724,13 +724,18 @@ INDEX tbl@a_idx  ALTER INDEX tbl@a_idx CONFIGURE ZONE USING
                  voter_constraints = '[+region=us-east-1]',
                  lease_preferences = '[[+region=us-east-1]]'
 
+# Drop the last region, making the database a non-multi-region database.
+# Then, modify the zone configs so that we can test transitioning to
+# multi-region with a zone config applied.
 statement ok
 ALTER DATABASE initial_multiregion_db DROP REGION "us-east-1";
 ALTER INDEX tbl@a_idx CONFIGURE ZONE USING num_replicas = 10
 
 statement error zone configuration for index tbl@a_idx has field "num_replicas" set which will be overwritten when setting the initial PRIMARY REGION\nHINT: discard the zone config using CONFIGURE ZONE DISCARD before continuing
 ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
 
+# Variations of tests where a zone config is present while we try to transition
+# to a multi-region database.
 statement ok
 ALTER INDEX tbl@a_idx CONFIGURE ZONE DISCARD;
 ALTER TABLE tbl CONFIGURE ZONE USING num_replicas = 10
@@ -740,11 +745,150 @@ ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
 
 statement ok
 ALTER TABLE tbl CONFIGURE ZONE DISCARD;
-ALTER DATABASE initial_multiregion_db CONFIGURE ZONE USING num_replicas = 10;
+ALTER DATABASE initial_multiregion_db CONFIGURE ZONE USING num_replicas = 10
 
 statement error zone configuration for database initial_multiregion_db has field "num_replicas" set which will be overwritten when setting the the initial PRIMARY REGION\nHINT: discard the zone config using CONFIGURE ZONE DISCARD before continuing
 ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
 
+# Validate that if we DISCARD the zone configuration, we can successfully turn
+# this database into a multi-region database.
 statement ok
 ALTER DATABASE initial_multiregion_db CONFIGURE ZONE DISCARD;
 ALTER DATABASE initial_multiregion_db SET PRIMARY REGION "us-east-1"
+
+# Test case where user has an applied zone config that won't directly get
+# overwritten by the locality transition.
+subtest force_repaving_all_zone_configs
+
+statement ok
+USE initial_multiregion_db;
+CREATE TABLE tbl1 (
+  pk INT PRIMARY KEY,
+  i INT,
+  INDEX(i)
+) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER INDEX tbl1@tbl1_i_idx CONFIGURE ZONE USING num_replicas=10;
+SET override_multi_region_zone_config = false
+
+statement error attempting to update zone config which contains an extra zone configuration for index tbl1@tbl1_i_idx with field num_replicas populated
+ALTER TABLE tbl1 SET LOCALITY GLOBAL
+
+# This statement should wipe out the zone configuration we applied on the
+# index above.
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl1 SET LOCALITY GLOBAL;
+SET override_multi_region_zone_config = false
+
+# Validate that the zone configuration is gone
+query TT
+SHOW ZONE CONFIGURATION FOR INDEX tbl1@tbl1_i_idx
+----
+TABLE tbl1  ALTER TABLE tbl1 CONFIGURE ZONE USING
+            range_min_bytes = 134217728,
+            range_max_bytes = 536870912,
+            gc.ttlseconds = 90000,
+            global_reads = true,
+            num_replicas = 3,
+            num_voters = 3,
+            constraints = '{+region=us-east-1: 1}',
+            voter_constraints = '[+region=us-east-1]',
+            lease_preferences = '[[+region=us-east-1]]'
+
+# Confirm that the zone configuration was wiped above by changing the locality
+# again, this time without an override.
+statement ok
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION
+
+# Repeat the test, this time with an alter to REGIONAL BY TABLE.
+statement ok
+ALTER TABLE tbl1 SET LOCALITY GLOBAL
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER INDEX tbl1@tbl1_i_idx CONFIGURE ZONE USING num_replicas=10;
+SET override_multi_region_zone_config = false
+
+statement error attempting to update zone config which contains an extra zone configuration for index tbl1@tbl1_i_idx with field num_replicas populated
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN "us-east-1"
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN "us-east-1";
+SET override_multi_region_zone_config = false
+
+statement ok
+ALTER TABLE tbl1 SET LOCALITY GLOBAL
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER INDEX tbl1@tbl1_i_idx CONFIGURE ZONE USING num_replicas=10;
+SET override_multi_region_zone_config = false
+
+statement error attempting to update zone config which contains an extra zone configuration for index tbl1@tbl1_i_idx with field num_replicas populated
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;
+SET override_multi_region_zone_config = false
+
+statement ok
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY ROW
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER INDEX tbl1@tbl1_i_idx CONFIGURE ZONE USING num_replicas=10;
+SET override_multi_region_zone_config = false
+
+statement error attempting to update zone config which contains an extra zone configuration for index tbl1@tbl1_i_idx with field num_replicas populated
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl1 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION;
+SET override_multi_region_zone_config = false
+
+# Now test to ensure that the same test holds true if we're transitioning from
+# REGIONAL BY ROW.
+statement ok
+CREATE TABLE tbl2 (
+  pk INT PRIMARY KEY,
+  i INT,
+  INDEX(i)
+) LOCALITY REGIONAL BY ROW
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER INDEX tbl2@tbl2_i_idx CONFIGURE ZONE USING num_replicas=10;
+SET override_multi_region_zone_config = false
+
+statement error attempting to update zone config which contains an extra zone configuration for index tbl2@tbl2_i_idx with field num_replicas populated
+ALTER TABLE tbl2 SET LOCALITY GLOBAL
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl2 SET LOCALITY GLOBAL;
+SET override_multi_region_zone_config = false
+
+statement ok
+ALTER TABLE tbl2 SET LOCALITY REGIONAL BY ROW
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl2 CONFIGURE ZONE USING num_replicas=10;
+SET override_multi_region_zone_config = false
+
+statement error attempting to update zone configuration for table tbl2 which contains modified field "num_replicas"
+ALTER TABLE tbl2 SET LOCALITY GLOBAL
+
+statement ok
+SET override_multi_region_zone_config = true;
+ALTER TABLE tbl2 SET LOCALITY GLOBAL;
+SET override_multi_region_zone_config = false
+
+statement ok
+ALTER TABLE tbl2 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION

pkg/ccl/multiregionccl/BUILD.bazel

@@ -37,6 +37,7 @@ go_test(
         "//pkg/ccl/utilccl",
         "//pkg/jobs",
         "//pkg/keys",
+        "//pkg/kv",
         "//pkg/roachpb",
         "//pkg/security",
         "//pkg/security/securitytest",

pkg/ccl/multiregionccl/regional_by_row_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/testutilsccl"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
@@ -343,12 +344,19 @@ func TestAlterTableLocalityRegionalByRowError(t *testing.T) {
 							params.Locality.Tiers = []roachpb.Tier{
 								{Key: "region", Value: "ajstorm-1"},
 							}
+							var sqlDB *gosql.DB
 							params.Knobs = base.TestingKnobs{
 								SQLSchemaChanger: &sql.SchemaChangerTestingKnobs{
 									BackfillChunkSize: chunkSize,
 								},
 								DistSQL: &execinfra.TestingKnobs{
 									RunBeforeBackfillChunk: func(sp roachpb.Span) error {
+										// Run a validate query on each chunk.
+										_, err := sqlDB.Exec(`SELECT crdb_internal.validate_multi_region_zone_configs()`)
+										if err != nil {
+											return errors.Wrap(err, "error validating zone configs during an in progress backfill")
+										}
+
 										currentBackfillChunk += 1
 										if currentBackfillChunk != alterState.cancelOnBackfillChunk {
 											return nil
@@ -357,7 +365,9 @@ func TestAlterTableLocalityRegionalByRowError(t *testing.T) {
 									},
 								},
 							}
-							s, sqlDB, kvDB := serverutils.StartServer(t, params)
+							var s serverutils.TestServerInterface
+							var kvDB *kv.DB
+							s, sqlDB, kvDB = serverutils.StartServer(t, params)
 							db = sqlDB
 							defer s.Stopper().Stop(ctx)
 

pkg/config/zonepb/zone.go

@@ -632,6 +632,7 @@ type DiffWithZoneMismatch struct {
 func (z *ZoneConfig) DiffWithZone(
 	other ZoneConfig, fieldList []tree.Name,
 ) (bool, DiffWithZoneMismatch, error) {
+	mismatchingNumReplicas := false
 	for _, fieldName := range fieldList {
 		switch fieldName {
 		case "num_replicas":
@@ -640,6 +641,13 @@ func (z *ZoneConfig) DiffWithZone(
 			}
 			if z.NumReplicas == nil || other.NumReplicas == nil ||
 				*z.NumReplicas != *other.NumReplicas {
+				// In cases where one of the zone configs are placeholders,
+				// defer the error reporting to below so that we can correctly
+				// report on a subzone difference, should one exist.
+				if z.IsSubzonePlaceholder() || other.IsSubzonePlaceholder() {
+					mismatchingNumReplicas = true
+					continue
+				}
 				return false, DiffWithZoneMismatch{
 					Field: "num_replicas",
 				}, nil
@@ -831,6 +839,13 @@ func (z *ZoneConfig) DiffWithZone(
 			}, nil
 		}
 	}
+	// If we've got a mismatch in the num_replicas field and we haven't found
+	// any other mismatch, report on num_replicas.
+	if mismatchingNumReplicas {
+		return false, DiffWithZoneMismatch{
+			Field: "num_replicas",
+		}, nil
+	}
 	return true, DiffWithZoneMismatch{}, nil
 }
 

pkg/sql/alter_table_locality.go

@@ -461,12 +461,10 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error {
 	)
 
 	// We should check index zone configs if moving to REGIONAL BY ROW.
-	checkIndexZoneConfigs := newLocality.LocalityLevel == tree.LocalityLevelRow
 	if err := params.p.validateZoneConfigForMultiRegionTableWasNotModifiedByUser(
 		params.ctx,
 		n.dbDesc,
 		n.tableDesc,
-		checkIndexZoneConfigs,
 	); err != nil {
 		return err
 	}

pkg/sql/catalog/descriptor.go

@@ -360,6 +360,8 @@ type TypeDescriptor interface {
 	PrimaryRegionName() (descpb.RegionName, error)
 	RegionNames() (descpb.RegionNames, error)
 	RegionNamesIncludingTransitioning() (descpb.RegionNames, error)
+	RegionNamesForValidation() (descpb.RegionNames, error)
+	TransitioningRegionNames() (descpb.RegionNames, error)
 }
 
 // TypeDescriptorResolver is an interface used during hydration of type

pkg/sql/catalog/multiregion/region_config.go

@@ -29,10 +29,11 @@ const minNumRegionsForSurviveRegionGoal = 3
 // inform be a RegionConfig must be made directly on those structs and a new
 // RegionConfig must be synthesized to pick up those changes.
 type RegionConfig struct {
-	survivalGoal  descpb.SurvivalGoal
-	regions       descpb.RegionNames
-	primaryRegion descpb.RegionName
-	regionEnumID  descpb.ID
+	survivalGoal         descpb.SurvivalGoal
+	regions              descpb.RegionNames
+	transitioningRegions descpb.RegionNames
+	primaryRegion        descpb.RegionName
+	regionEnumID         descpb.ID
 }
 
 // SurvivalGoal returns the survival goal configured on the RegionConfig.
@@ -65,24 +66,46 @@ func (r RegionConfig) PrimaryRegionString() string {
 	return string(r.PrimaryRegion())
 }
 
+// TransitioningRegions returns all the regions which are currently transitioning
+// from or to being PUBLIC.
+func (r RegionConfig) TransitioningRegions() descpb.RegionNames {
+	return r.transitioningRegions
+}
+
 // RegionEnumID returns the multi-region enum ID.
 func (r *RegionConfig) RegionEnumID() descpb.ID {
 	return r.regionEnumID
 }
 
+// MakeRegionConfigOption is an option for MakeRegionConfig
+type MakeRegionConfigOption func(r *RegionConfig)
+
+// WithTransitioningRegions is an option to include transitioning
+// regions into MakeRegionConfig.
+func WithTransitioningRegions(transitioningRegions descpb.RegionNames) MakeRegionConfigOption {
+	return func(r *RegionConfig) {
+		r.transitioningRegions = transitioningRegions
+	}
+}
+
 // MakeRegionConfig constructs a RegionConfig.
 func MakeRegionConfig(
 	regions descpb.RegionNames,
 	primaryRegion descpb.RegionName,
 	survivalGoal descpb.SurvivalGoal,
 	regionEnumID descpb.ID,
+	opts ...MakeRegionConfigOption,
 ) RegionConfig {
-	return RegionConfig{
+	ret := RegionConfig{
 		regions:       regions,
 		primaryRegion: primaryRegion,
 		survivalGoal:  survivalGoal,
 		regionEnumID:  regionEnumID,
 	}
+	for _, opt := range opts {
+		opt(&ret)
+	}
+	return ret
 }
 
 // canSatisfySurvivalGoal returns true if the survival goal is satisfiable by

pkg/sql/catalog/typedesc/type_desc.go

@@ -196,14 +196,33 @@ func (desc *Immutable) RegionNames() (descpb.RegionNames, error) {
 	return regions, nil
 }
 
-// RegionNamesForZoneConfigValidation returns all regions on the multi-region
-// enum to make validation with the public zone configs possible. Since the zone
-// configs are only updated when a transaction commits, this must ignore all
-// regions being added (since they will not be reflected in the zone
-// configuration yet), but it must include all region being dropped (since they
-// will not be dropped from the zone configuration until they are fully removed
-// from the type descriptor, again, at the end of the transaction).
-func (desc *Immutable) RegionNamesForZoneConfigValidation() (descpb.RegionNames, error) {
+// TransitioningRegionNames returns regions which are transitioning to PUBLIC
+// or are being removed.
+func (desc *Immutable) TransitioningRegionNames() (descpb.RegionNames, error) {
+	if desc.Kind != descpb.TypeDescriptor_MULTIREGION_ENUM {
+		return nil, errors.AssertionFailedf(
+			"can not get regions of a non multi-region enum %d", desc.ID,
+		)
+	}
+	var regions descpb.RegionNames
+	for _, member := range desc.EnumMembers {
+		if member.Direction != descpb.TypeDescriptor_EnumMember_NONE {
+			regions = append(regions, descpb.RegionName(member.LogicalRepresentation))
+		}
+	}
+	return regions, nil
+}
+
+// RegionNamesForValidation returns all regions on the multi-region
+// enum to make validation with the public zone configs and partitons
+// possible.
+// Since the partitions and zone configs are only updated when a transaction
+// commits, this must ignore all regions being added (since they will not be
+// reflected in the zone configuration yet), but it must include all region
+// being dropped (since they will not be dropped from the zone configuration
+// until they are fully removed from the type descriptor, again, at the end
+// of the transaction).
+func (desc *Immutable) RegionNamesForValidation() (descpb.RegionNames, error) {
 	if desc.Kind != descpb.TypeDescriptor_MULTIREGION_ENUM {
 		return nil, errors.AssertionFailedf(
 			"can not get regions of a non multi-region enum %d", desc.ID,