sql: stop duplicating regions on the database descriptor

Previously, we were duplicating regions both on the type descriptor and the database descriptor. Theres a verbose explanation about why this is bad on the linked issue, but the crux of the problem with this approach is the added complexity it requires keeping these two lists in sync as regions are added/dropped from a database in the face of possible failure scenarios. This patch removes the `Regions` field from the database descriptors's region config. Instead, the regions stored on the multi-region enum serve as the sole source of truth. To accomplish this, we introduce a new `RegionConfig` struct which is synthesized from the state (primary region, survival goal) stored on the database and the state (available regions) stored on the type descriptor. All the places that constructed zone configurations from the raw region config proto now use this new struct. This patch also introduces a `InitializationRegionConfig` struct which wraps a `RegionConfig` with initilization specific meta-data. I'm not too thrilled by the fact that survival goal and primary region is still stored on the database descriptor. Ideally, the database descriptor would contain just the multi-region enum ID and all user configured parameters would be in one place (on the type descriptor's region config). The level of indirection introduced by this patch sets us on the path to that future. Closes #60620 Release justification: low risk updates to new functionality Release note: None
cockroachdb · Mar 16, 2021 · 861ff91 · 861ff91
1 parent 98ae002
commit 861ff91
Show file tree

Hide file tree

Showing 37 changed files with 1,273 additions and 1,383 deletions.
diff --git a/pkg/ccl/backupccl/BUILD.bazel b/pkg/ccl/backupccl/BUILD.bazel
@@ -57,6 +57,7 @@ go_library(
         "//pkg/sql/catalog/dbdesc",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
+        "//pkg/sql/catalog/multiregion",
         "//pkg/sql/catalog/schemadesc",
         "//pkg/sql/catalog/schemaexpr",
         "//pkg/sql/catalog/systemschema",

diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go
@@ -32,6 +32,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
@@ -376,7 +377,7 @@ func WriteDescriptors(
 			if err != nil {
 				return err
 			}
-			if dbDesc.GetRegionConfig() != nil {
+			if dbDesc.IsMultiRegion() {
 				if table.GetLocalityConfig() == nil {
 					table.(*tabledesc.Mutable).SetTableLocalityRegionalByTable(tree.PrimaryRegionNotSpecifiedName)
 				}
@@ -1086,7 +1087,7 @@ func createImportingDescriptors(
 				// as the system tables are restored.
 				mrEnumsFound := make(map[descpb.ID]descpb.ID)
 				for _, t := range typesByID {
-					typeDesc := t.TypeDesc()
+					typeDesc := typedesc.NewBuilder(t.TypeDesc()).BuildImmutableType()
 					if typeDesc.GetKind() == descpb.TypeDescriptor_MULTIREGION_ENUM {
 						// Check to see if we've found more than one multi-region enum on any
 						// given database.
@@ -1111,10 +1112,20 @@ func createImportingDescriptors(
 							// configuration.
 							if details.DescriptorCoverage != tree.AllDescriptors {
 								log.Infof(ctx, "restoring zone configuration for database %d", desc.ID)
+								regionNames, err := typeDesc.RegionNames()
+								if err != nil {
+									return err
+								}
+								regionConfig := multiregion.NewRegionConfig(
+									regionNames,
+									desc.RegionConfig.PrimaryRegion,
+									desc.RegionConfig.SurvivalGoal,
+									desc.RegionConfig.RegionEnumID,
+								)
 								if err := sql.ApplyZoneConfigFromDatabaseRegionConfig(
 									ctx,
 									desc.GetID(),
-									*desc.RegionConfig,
+									regionConfig,
 									txn,
 									p.ExecCfg(),
 								); err != nil {
@@ -1241,11 +1252,15 @@ func createImportingDescriptors(
 								return err
 							}
 
+							regionConfig, err := sql.SynthesizeRegionConfig(ctx, txn, desc.ID, descsCol)
+							if err != nil {
+								return err
+							}
 							if err := sql.ApplyZoneConfigForMultiRegionTable(
 								ctx,
 								txn,
 								p.ExecCfg(),
-								*desc.RegionConfig,
+								regionConfig,
 								mutTable,
 								sql.ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes,
 							); err != nil {

diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go
@@ -597,7 +597,7 @@ func allocateDescriptorRewrites(
 				// want to modify the table so that it can exist in the multi-region
 				// database.
 				// https://github.com/cockroachdb/cockroach/issues/59804
-				if parentDB.GetRegionConfig() != nil {
+				if parentDB.IsMultiRegion() {
 					return pgerror.Newf(pgcode.FeatureNotSupported,
 						"cannot restore individual table %d into multi-region database %d",
 						table.GetID(),

diff --git a/pkg/ccl/importccl/import_table_creation.go b/pkg/ccl/importccl/import_table_creation.go
@@ -164,7 +164,7 @@ func MakeSimpleTableDescriptor(
 		parentID,
 		parentSchemaID,
 		tableID,
-		nil, /* regionConfig */
+		nil,              /* regionConfig */
 		hlc.Timestamp{WallTime: walltime},
 		descpb.NewDefaultPrivilegeDescriptor(security.AdminRoleName()),
 		affected,
@@ -217,10 +217,9 @@ func (so *importSequenceOperators) GetSerialSequenceNameFromColumn(
 }
 
 // CurrentDatabaseRegionConfig is part of the tree.EvalDatabase interface.
-func (so *importSequenceOperators) CurrentDatabaseRegionConfig() (
-	tree.DatabaseRegionConfig,
-	error,
-) {
+func (so *importSequenceOperators) CurrentDatabaseRegionConfig(
+	_ context.Context,
+) (tree.DatabaseRegionConfig, error) {
 	return nil, errors.WithStack(errSequenceOperators)
 }
 

diff --git a/pkg/ccl/multiregionccl/BUILD.bazel b/pkg/ccl/multiregionccl/BUILD.bazel
@@ -11,6 +11,7 @@ go_library(
         "//pkg/sql",
         "//pkg/sql/catalog/catalogkv",
         "//pkg/sql/catalog/descpb",
+        "//pkg/sql/catalog/multiregion",
         "//pkg/sql/catalog/typedesc",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",

diff --git a/pkg/ccl/multiregionccl/multiregion.go b/pkg/ccl/multiregionccl/multiregion.go
@@ -17,28 +17,29 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/multiregion"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 )
 
 func init() {
-	sql.CreateRegionConfigCCL = createRegionConfig
+	sql.InitializeMultiRegionMetadataCCL = initializeMultiRegionMetadata
 	sql.GetMultiRegionEnumAddValuePlacementCCL = getMultiRegionEnumAddValuePlacement
 }
 
-func createRegionConfig(
+func initializeMultiRegionMetadata(
 	ctx context.Context,
 	evalCtx *tree.EvalContext,
 	execCfg *sql.ExecutorConfig,
 	liveRegions sql.LiveClusterRegions,
-	survivalGoal tree.SurvivalGoal,
-	primaryRegion tree.Name,
+	goal tree.SurvivalGoal,
+	primaryRegion descpb.RegionName,
 	regions []tree.Name,
-) (descpb.DatabaseDescriptor_RegionConfig, error) {
+) (*multiregion.RegionConfig, error) {
 	if err := checkClusterSupportsMultiRegion(evalCtx); err != nil {
-		return descpb.DatabaseDescriptor_RegionConfig{}, err
+		return nil, err
 	}
 
 	if err := utilccl.CheckEnterpriseEnabled(
@@ -47,78 +48,66 @@ func createRegionConfig(
 		execCfg.Organization(),
 		"multi-region features",
 	); err != nil {
-		return descpb.DatabaseDescriptor_RegionConfig{}, err
+		return nil, err
 	}
 
-	var regionConfig descpb.DatabaseDescriptor_RegionConfig
-	var err error
-	regionConfig.SurvivalGoal, err = sql.TranslateSurvivalGoal(survivalGoal)
+	survivalGoal, err := sql.TranslateSurvivalGoal(goal)
 	if err != nil {
-		return descpb.DatabaseDescriptor_RegionConfig{}, err
+		return nil, err
 	}
 
-	regionConfig.PrimaryRegion = descpb.RegionName(primaryRegion)
-	if regionConfig.PrimaryRegion != descpb.RegionName(tree.PrimaryRegionNotSpecifiedName) {
-		if err := sql.CheckLiveClusterRegion(liveRegions, regionConfig.PrimaryRegion); err != nil {
-			return descpb.DatabaseDescriptor_RegionConfig{}, err
+	if primaryRegion != descpb.RegionName(tree.PrimaryRegionNotSpecifiedName) {
+		if err := sql.CheckClusterRegionIsLive(liveRegions, primaryRegion); err != nil {
+			return nil, err
 		}
 	}
+	regionNames := make(descpb.RegionNames, 0, len(regions)+1)
+	seenRegions := make(map[descpb.RegionName]struct{}, len(regions)+1)
 	if len(regions) > 0 {
-		if regionConfig.PrimaryRegion == descpb.RegionName(tree.PrimaryRegionNotSpecifiedName) {
-			return descpb.DatabaseDescriptor_RegionConfig{}, pgerror.Newf(
+		if primaryRegion == descpb.RegionName(tree.PrimaryRegionNotSpecifiedName) {
+			return nil, pgerror.Newf(
 				pgcode.InvalidDatabaseDefinition,
 				"PRIMARY REGION must be specified if REGIONS are specified",
 			)
 		}
-		regionConfig.Regions = make([]descpb.DatabaseDescriptor_RegionConfig_Region, 0, len(regions)+1)
-		seenRegions := make(map[descpb.RegionName]struct{}, len(regions)+1)
 		for _, r := range regions {
 			region := descpb.RegionName(r)
-			if err := sql.CheckLiveClusterRegion(liveRegions, region); err != nil {
-				return descpb.DatabaseDescriptor_RegionConfig{}, err
+			if err := sql.CheckClusterRegionIsLive(liveRegions, region); err != nil {
+				return nil, err
 			}
 
 			if _, ok := seenRegions[region]; ok {
-				return descpb.DatabaseDescriptor_RegionConfig{}, pgerror.Newf(
+				return nil, pgerror.Newf(
 					pgcode.InvalidName,
 					"region %q defined multiple times",
 					region,
 				)
 			}
 			seenRegions[region] = struct{}{}
-			regionConfig.Regions = append(
-				regionConfig.Regions,
-				descpb.DatabaseDescriptor_RegionConfig_Region{
-					Name: region,
-				},
-			)
-		}
-		// If PRIMARY REGION is not in REGIONS, add it implicitly.
-		if _, ok := seenRegions[regionConfig.PrimaryRegion]; !ok {
-			regionConfig.Regions = append(
-				regionConfig.Regions,
-				descpb.DatabaseDescriptor_RegionConfig_Region{
-					Name: regionConfig.PrimaryRegion,
-				},
-			)
-		}
-		sort.SliceStable(regionConfig.Regions, func(i, j int) bool {
-			return regionConfig.Regions[i].Name < regionConfig.Regions[j].Name
-		})
-	} else {
-		regionConfig.Regions = []descpb.DatabaseDescriptor_RegionConfig_Region{
-			{Name: regionConfig.PrimaryRegion},
+			regionNames = append(regionNames, region)
 		}
 	}
+	// If PRIMARY REGION is not in REGIONS, add it implicitly.
+	if _, ok := seenRegions[primaryRegion]; !ok {
+		regionNames = append(regionNames, primaryRegion)
+	}
+
+	sort.SliceStable(regionNames, func(i, j int) bool {
+		return regionNames[i] < regionNames[j]
+	})
 
 	// Generate a unique ID for the multi-region enum type descriptor here as
 	// well.
-	id, err := catalogkv.GenerateUniqueDescID(ctx, execCfg.DB, execCfg.Codec)
+	regionEnumID, err := catalogkv.GenerateUniqueDescID(ctx, execCfg.DB, execCfg.Codec)
 	if err != nil {
-		return descpb.DatabaseDescriptor_RegionConfig{}, err
+		return nil, err
 	}
-	regionConfig.RegionEnumID = id
-	return regionConfig, nil
+	return multiregion.NewRegionConfig(
+		regionNames,
+		primaryRegion,
+		survivalGoal,
+		regionEnumID,
+	), nil
 }
 
 func checkClusterSupportsMultiRegion(evalCtx *tree.EvalContext) error {

diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel
@@ -277,6 +277,7 @@ go_library(
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/hydratedtables",
         "//pkg/sql/catalog/lease",
+        "//pkg/sql/catalog/multiregion",
         "//pkg/sql/catalog/resolver",
         "//pkg/sql/catalog/schemadesc",
         "//pkg/sql/catalog/schemaexpr",
@@ -526,6 +527,7 @@ go_test(
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/lease",
+        "//pkg/sql/catalog/multiregion",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/catalog/typedesc",
         "//pkg/sql/distsql",