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 cockroachdb#60620

Release justification: low risk updates to new functionality
Release note: None

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",

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 {

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(),

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)
 }
 

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",

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 {

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",