From ee5bd70917b9b4b372e34f3263c011421ef8c350 Mon Sep 17 00:00:00 2001 From: Adam Storm Date: Fri, 9 Apr 2021 15:47:51 -0400 Subject: [PATCH 1/6] sql: Clear all multi-region fields on table locality changes In the past there were cases where a table locality change (even one where override was specified) would not clear all of the multi-region zone configuration fields for all zone configurations. The simplest example is if you had a zone configuration manually specified by the user on an index and you were ALTERing the table locality to REGIONAL BY TABLE or GLOBAL. Since neither of these transitions look at the index zone configuration, it would remain untouched, and future locality changes would still require the override. This commit change the behavior such that if override is specified, all multi-region fields of the zone configuration will be cleared and a future table locality change will not require override. Release note: None --- .../logic_test/multi_region_zone_configs | 130 +++++++++++++++++- pkg/sql/region_util.go | 22 ++- 2 files changed, 150 insertions(+), 2 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs index 4ae3480e392f..95ddafd6d6fa 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs @@ -724,6 +724,9 @@ 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 @@ -731,6 +734,8 @@ 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,134 @@ 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 configuration for table tbl1 which contains modified field "num_replicas" +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 TABLE IN PRIMARY REGION; diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 521a8fbd29f8..bc5bbac1bb7a 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -507,6 +507,25 @@ var ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes = func( if err != nil { return false, zonepb.ZoneConfig{}, err } + + // Wipe out the subzone multi-region fields before we copy over the + // multi-region fields to the zone config down below. We have to do this to + // handle the case where users have set a zone config on an index and we're + // ALTERing to a table locality that doesn't lay down index zone + // configurations (e.g. GLOBAL or REGIONAL BY TABLE). Since the user will + // have to override to perform the ALTER, we want to wipe out the index + // zone config so that the user won't have to override again the next time + // the want to ALTER the table locality. + newSubzones := zc.Subzones[:0] + for _, sz := range zc.Subzones { + sz.Config.CopyFromZone(zonepb.ZoneConfig{}, zonepb.MultiRegionZoneConfigFields) + // If we haven't emptied out the subzone, append it to the new slice. + if !sz.Config.Equal(zonepb.ZoneConfig{}) && !sz.Config.Equal(zonepb.NewZoneConfig()) { + newSubzones = append(newSubzones, sz) + } + } + zc.Subzones = newSubzones + zc.CopyFromZone(*localityZoneConfig, zonepb.MultiRegionZoneConfigFields) hasNewSubzones := table.IsLocalityRegionalByRow() @@ -617,7 +636,8 @@ func ApplyZoneConfigForMultiRegionTable( return err } } else if deleteZoneConfig { - // Delete the zone configuration if it exists but the new zone config is blank. + // Delete the zone configuration if it exists but the new zone config is + // blank. if _, err = execCfg.InternalExecutor.Exec( ctx, "delete-zone-multiregion-table", From 417731f3e3459d2b69497c7fd0a163a4c43c8e58 Mon Sep 17 00:00:00 2001 From: Adam Storm Date: Sun, 11 Apr 2021 13:30:26 -0400 Subject: [PATCH 2/6] sql: Improve error message when user's modified MR zone config Previously we were returning an inaccurate error message when a user modified zone config was present on a multi-region table. This was due to the fact that we'd key off of num_replicas being set in a placeholder zone config and think that that was the difference between the two zone configs. In actuality, the difference was in the subzones. This commit allows for the reporting of the true difference, the presence of the index in the subzones. Release note: None --- .../logic_test/multi_region_zone_configs | 24 +++++++++++++++---- pkg/config/zonepb/zone.go | 15 ++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs index 95ddafd6d6fa..9d70e049b984 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs @@ -773,7 +773,7 @@ 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 configuration for table tbl1 which contains modified field "num_replicas" +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 @@ -859,7 +859,7 @@ CREATE TABLE tbl2 ( pk INT PRIMARY KEY, i INT, INDEX(i) -) LOCALITY REGIONAL BY ROW; +) LOCALITY REGIONAL BY ROW statement ok SET override_multi_region_zone_config = true; @@ -867,12 +867,28 @@ 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; +SET override_multi_region_zone_config = false statement ok -ALTER TABLE tbl2 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION; +ALTER TABLE tbl2 SET LOCALITY REGIONAL BY TABLE IN PRIMARY REGION diff --git a/pkg/config/zonepb/zone.go b/pkg/config/zonepb/zone.go index f5300eaecaef..7e3a810ea9ab 100644 --- a/pkg/config/zonepb/zone.go +++ b/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 } From ada4d4db2c027664a0f95af8bf4a6eb6f002f6df Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 9 Apr 2021 14:01:21 +1000 Subject: [PATCH 3/6] sql: fix zone config validation during a REGIONAL BY ROW transition Involves peeping into the mutations and excluding the new indexes being created -- these will not have the correct zone configs set on them. Release note (bug fix): Fix a bug where crdb_internal.validate_multi_region_zone_configs() fails during a REGIONAL BY ROW locality transition. --- pkg/ccl/multiregionccl/BUILD.bazel | 1 + .../multiregionccl/regional_by_row_test.go | 12 +++- pkg/sql/region_util.go | 59 +++++++++++++++---- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/pkg/ccl/multiregionccl/BUILD.bazel b/pkg/ccl/multiregionccl/BUILD.bazel index 41ddf93b580b..c7996ec7c658 100644 --- a/pkg/ccl/multiregionccl/BUILD.bazel +++ b/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", diff --git a/pkg/ccl/multiregionccl/regional_by_row_test.go b/pkg/ccl/multiregionccl/regional_by_row_test.go index 631196bd09ee..8554fcdb2a7b 100644 --- a/pkg/ccl/multiregionccl/regional_by_row_test.go +++ b/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) diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index bc5bbac1bb7a..f3afd27adf0c 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -1484,26 +1484,65 @@ func (p *planner) validateZoneConfigForMultiRegionTable( return err } + // When there is a transition to/from REGIONAL BY ROW, the new indexes + // being set up will have zone configs which mismatch with the old + // table locality config. As we validate against the old table locality + // config (as the new indexes are not swapped in yet), exclude these + // indexes from any zone configuration validation. + regionalByRowNewIndexes := make(map[uint32]struct{}) + for _, mut := range desc.AllMutations() { + if pkSwap := mut.AsPrimaryKeySwap(); pkSwap != nil { + swapDesc := pkSwap.PrimaryKeySwapDesc() + if swapDesc.LocalityConfigSwap != nil { + for _, id := range swapDesc.NewIndexes { + regionalByRowNewIndexes[uint32(id)] = struct{}{} + } + regionalByRowNewIndexes[uint32(swapDesc.NewPrimaryIndexId)] = struct{}{} + } + // There can only be one pkSwap at a time, so break now. + break + } + } + // Some inactive subzones may remain on the zone configuration until it is cleaned up - // at a later step. Keep track of all non-drop indexes from the descriptor. - activeSubzoneIndexIDs := make(map[uint32]tree.Name, len(desc.NonDropIndexes())) + // at a later step. Remove these as well as the regional by row new indexes. + subzoneIndexIDsToDiff := make(map[uint32]tree.Name, len(desc.NonDropIndexes())) for _, idx := range desc.NonDropIndexes() { - activeSubzoneIndexIDs[uint32(idx.GetID())] = tree.Name(idx.GetName()) + if _, ok := regionalByRowNewIndexes[uint32(idx.GetID())]; !ok { + subzoneIndexIDsToDiff[uint32(idx.GetID())] = tree.Name(idx.GetName()) + } } - // Remove inactive subzones from the comparison. - filteredSubzones := currentZoneConfig.Subzones[:0] + // We only want to compare against the list of subzones on active indexes, + // so filter the subzone list based on the subzoneIndexIDsToDiff computed above. + filteredCurrentZoneConfigSubzones := currentZoneConfig.Subzones[:0] for _, c := range currentZoneConfig.Subzones { - if _, ok := activeSubzoneIndexIDs[c.IndexID]; ok { - filteredSubzones = append(filteredSubzones, c) + if _, ok := subzoneIndexIDsToDiff[c.IndexID]; ok { + filteredCurrentZoneConfigSubzones = append(filteredCurrentZoneConfigSubzones, c) } } - currentZoneConfig.Subzones = filteredSubzones + currentZoneConfig.Subzones = filteredCurrentZoneConfigSubzones // Strip the placeholder status if there are no active subzones on the current // zone config. - if len(filteredSubzones) == 0 && currentZoneConfig.IsSubzonePlaceholder() { + if len(filteredCurrentZoneConfigSubzones) == 0 && currentZoneConfig.IsSubzonePlaceholder() { currentZoneConfig.NumReplicas = nil } + + // Remove regional by row new indexes from the expected zone config. + // These will be incorrect as ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes + // will apply the existing locality config on them instead of the + // new locality config. + filteredExpectedZoneConfigSubzones := expectedZoneConfig.Subzones[:0] + for _, c := range expectedZoneConfig.Subzones { + if _, ok := regionalByRowNewIndexes[c.IndexID]; !ok { + filteredExpectedZoneConfigSubzones = append( + filteredExpectedZoneConfigSubzones, + c, + ) + } + } + expectedZoneConfig.Subzones = filteredExpectedZoneConfigSubzones + // Mark the expected NumReplicas as 0 if we have a placeholder // and the current zone config is also a placeholder. // The latter check is required as in cases where non-multiregion fields @@ -1530,7 +1569,7 @@ func (p *planner) validateZoneConfigForMultiRegionTable( descType := "table" name := tableName.String() if mismatch.IndexID != 0 { - indexName, ok := activeSubzoneIndexIDs[mismatch.IndexID] + indexName, ok := subzoneIndexIDsToDiff[mismatch.IndexID] if !ok { return errors.AssertionFailedf( "unexpected unknown index id %d on table %s (mismatch %#v)", From 8457452cb939f2e4e9201d132bd034f47960e38b Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Tue, 13 Apr 2021 09:43:08 +1000 Subject: [PATCH 4/6] sql: remove transitioning regions from zone config validation Transitioning regions can result in transient state which is hard to reason about and validate during a concurrent operation with REGIONAL BY ROW changes. To alleviate this, we relax the validation by removing the transitioning regions from the validation path. Steps: * Change RegionConfig to optionally include transitioning regions. * Make SynthesizeRegionConfig include transitioning regions. * Change the validation interface to surface transitioning regions. Remove this from regional by row zone config validation. Release note: None --- pkg/sql/alter_table_locality.go | 2 - pkg/sql/catalog/descriptor.go | 2 + pkg/sql/catalog/multiregion/region_config.go | 35 ++++++- pkg/sql/catalog/typedesc/type_desc.go | 35 +++++-- pkg/sql/region_util.go | 101 ++++++++++++------- 5 files changed, 125 insertions(+), 50 deletions(-) diff --git a/pkg/sql/alter_table_locality.go b/pkg/sql/alter_table_locality.go index f8b16b9a2a75..c0c19689ccf3 100644 --- a/pkg/sql/alter_table_locality.go +++ b/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 } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 4b2502aa0236..cb7671db859b 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/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 diff --git a/pkg/sql/catalog/multiregion/region_config.go b/pkg/sql/catalog/multiregion/region_config.go index 100f45bcc85e..779c7a049566 100644 --- a/pkg/sql/catalog/multiregion/region_config.go +++ b/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,48 @@ 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) + +// MakeRegionConfigOptionTransitioningRegions is an option to include transitioning +// regions into MakeRegionConfig. +func MakeRegionConfigOptionTransitioningRegions( + 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 diff --git a/pkg/sql/catalog/typedesc/type_desc.go b/pkg/sql/catalog/typedesc/type_desc.go index 3c220007f23d..85c0bf97a572 100644 --- a/pkg/sql/catalog/typedesc/type_desc.go +++ b/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, diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index f3afd27adf0c..906daa3ddd6f 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -967,10 +967,14 @@ func SynthesizeRegionConfigOffline( ) } -// SynthesizeRegionConfig is the public function for the synthesizing region -// configs in the common case (i.e. not the offline case). See -// synthesizeRegionConfig for more details on what it does under the covers. -func SynthesizeRegionConfig( +// SynthesizeRegionConfigForZoneConfigValidation returns a RegionConfig +// representing the user configured state of a multi-region database by +// coalescing state from both the database descriptor and multi-region type +// descriptor. It avoids the cache and is intended for use by DDL statements. +// Since it is intended to be called for validation of the RegionConfig against +// the current database zone configuration, it omits regions that are in the +// adding state, but includes those that are being dropped. +func SynthesizeRegionConfigForZoneConfigValidation( ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, ) (multiregion.RegionConfig, error) { return synthesizeRegionConfigImpl( @@ -979,18 +983,14 @@ func SynthesizeRegionConfig( dbID, descsCol, false, /* includeOffline */ - false, /* forZoneConfigValidate */ + true, /* forZoneConfigValidate */ ) } -// SynthesizeRegionConfigForZoneConfigValidation returns a RegionConfig -// representing the user configured state of a multi-region database by -// coalescing state from both the database descriptor and multi-region type -// descriptor. It avoids the cache and is intended for use by DDL statements. -// Since it is intended to be called for validation of the RegionConfig against -// the current database zone configuration, it omits regions that are in the -// adding state, but includes those that are being dropped. -func SynthesizeRegionConfigForZoneConfigValidation( +// SynthesizeRegionConfig is the public function for the synthesizing region +// configs in the common case (i.e. not the offline case). See +// synthesizeRegionConfig for more details on what it does under the covers. +func SynthesizeRegionConfig( ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, ) (multiregion.RegionConfig, error) { return synthesizeRegionConfigImpl( @@ -999,17 +999,14 @@ func SynthesizeRegionConfigForZoneConfigValidation( dbID, descsCol, false, /* includeOffline */ - true, /* forZoneConfigValidate */ + false, /* forZoneConfigValidate */ ) } // SynthesizeRegionConfigImpl returns a RegionConfig representing the user // configured state of a multi-region database by coalescing state from both // the database descriptor and multi-region type descriptor. It avoids the cache -// and is intended for use by DDL statements. It can be called either for a -// traditional construction, which omits all regions in the non-PUBLIC state, or -// for zone configuration validation, which only omits region that are being -// added. +// and is intended for use by DDL statements. func synthesizeRegionConfigImpl( ctx context.Context, txn *kv.Txn, @@ -1047,9 +1044,10 @@ func synthesizeRegionConfigImpl( if err != nil { return multiregion.RegionConfig{}, err } + var regionNames descpb.RegionNames if forZoneConfigValidate { - regionNames, err = regionEnum.RegionNamesForZoneConfigValidation() + regionNames, err = regionEnum.RegionNamesForValidation() } else { regionNames, err = regionEnum.RegionNames() } @@ -1057,11 +1055,17 @@ func synthesizeRegionConfigImpl( return regionConfig, err } + transitioningRegionNames, err := regionEnum.TransitioningRegionNames() + if err != nil { + return regionConfig, err + } + regionConfig = multiregion.MakeRegionConfig( regionNames, dbDesc.RegionConfig.PrimaryRegion, dbDesc.RegionConfig.SurvivalGoal, regionEnumID, + multiregion.MakeRegionConfigOptionTransitioningRegions(transitioningRegionNames), ) if err := multiregion.ValidateRegionConfig(regionConfig); err != nil { @@ -1151,6 +1155,7 @@ func (p *planner) CheckZoneConfigChangePermittedForMultiRegion( type zoneConfigForMultiRegionValidator interface { getExpectedDatabaseZoneConfig() (zonepb.ZoneConfig, error) getExpectedTableZoneConfig(desc catalog.TableDescriptor) (zonepb.ZoneConfig, error) + transitioningRegions() descpb.RegionNames newMismatchFieldError(descType string, descName string, field string) error newMissingSubzoneError(descType string, descName string, field string) error @@ -1171,6 +1176,11 @@ func (v *zoneConfigForMultiRegionValidatorSetInitialRegion) getExpectedDatabaseZ return *zonepb.NewZoneConfig(), nil } +func (v *zoneConfigForMultiRegionValidatorSetInitialRegion) transitioningRegions() descpb.RegionNames { + // There are no transitioning regions at setup time. + return nil +} + func (v *zoneConfigForMultiRegionValidatorSetInitialRegion) getExpectedTableZoneConfig( desc catalog.TableDescriptor, ) (zonepb.ZoneConfig, error) { @@ -1257,6 +1267,10 @@ func (v *zoneConfigForMultiRegionValidatorExistingMultiRegionObject) getExpected return expectedZoneConfig, err } +func (v *zoneConfigForMultiRegionValidatorExistingMultiRegionObject) transitioningRegions() descpb.RegionNames { + return v.regionConfig.TransitioningRegions() +} + // zoneConfigForMultiRegionValidatorModifiedByUser implements // interface zoneConfigForMultiRegionValidator. type zoneConfigForMultiRegionValidatorModifiedByUser struct { @@ -1433,10 +1447,7 @@ func (p *planner) validateZoneConfigForMultiRegionDatabase( // the user about that before it occurs (and require the // override_multi_region_zone_config session variable to be set). func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( - ctx context.Context, - dbDesc *dbdesc.Immutable, - desc *tabledesc.Mutable, - checkIndexZoneConfigs bool, + ctx context.Context, dbDesc catalog.DatabaseDescriptor, desc *tabledesc.Mutable, ) error { // If the user is overriding, or this is not a multi-region table our work here // is done. @@ -1447,7 +1458,7 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( if err != nil { return err } - regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, dbDesc.ID, p.Descriptors()) + regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, dbDesc.GetID(), p.Descriptors()) if err != nil { return err } @@ -1504,7 +1515,7 @@ func (p *planner) validateZoneConfigForMultiRegionTable( } } - // Some inactive subzones may remain on the zone configuration until it is cleaned up + // Some transitioning subzones may remain on the zone configuration until it is cleaned up // at a later step. Remove these as well as the regional by row new indexes. subzoneIndexIDsToDiff := make(map[uint32]tree.Name, len(desc.NonDropIndexes())) for _, idx := range desc.NonDropIndexes() { @@ -1513,13 +1524,27 @@ func (p *planner) validateZoneConfigForMultiRegionTable( } } - // We only want to compare against the list of subzones on active indexes, - // so filter the subzone list based on the subzoneIndexIDsToDiff computed above. + // Do not compare partitioning for these regions, as they may be in a + // transitioning state. + transitioningRegions := make(map[string]struct{}, len(zoneConfigForMultiRegionValidator.transitioningRegions())) + for _, transitioningRegion := range zoneConfigForMultiRegionValidator.transitioningRegions() { + transitioningRegions[string(transitioningRegion)] = struct{}{} + } + + // We only want to compare against the list of subzones on active indexes + // and partitions, so filter the subzone list based on the + // subzoneIndexIDsToDiff computed above. filteredCurrentZoneConfigSubzones := currentZoneConfig.Subzones[:0] for _, c := range currentZoneConfig.Subzones { - if _, ok := subzoneIndexIDsToDiff[c.IndexID]; ok { - filteredCurrentZoneConfigSubzones = append(filteredCurrentZoneConfigSubzones, c) + if c.PartitionName != "" { + if _, ok := transitioningRegions[c.PartitionName]; ok { + continue + } + } + if _, ok := subzoneIndexIDsToDiff[c.IndexID]; !ok { + continue } + filteredCurrentZoneConfigSubzones = append(filteredCurrentZoneConfigSubzones, c) } currentZoneConfig.Subzones = filteredCurrentZoneConfigSubzones // Strip the placeholder status if there are no active subzones on the current @@ -1528,18 +1553,24 @@ func (p *planner) validateZoneConfigForMultiRegionTable( currentZoneConfig.NumReplicas = nil } - // Remove regional by row new indexes from the expected zone config. + // Remove regional by row new indexes and transitioning partitions from the expected zone config. // These will be incorrect as ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes // will apply the existing locality config on them instead of the // new locality config. filteredExpectedZoneConfigSubzones := expectedZoneConfig.Subzones[:0] for _, c := range expectedZoneConfig.Subzones { - if _, ok := regionalByRowNewIndexes[c.IndexID]; !ok { - filteredExpectedZoneConfigSubzones = append( - filteredExpectedZoneConfigSubzones, - c, - ) + if c.PartitionName != "" { + if _, ok := transitioningRegions[c.PartitionName]; ok { + continue + } + } + if _, ok := regionalByRowNewIndexes[c.IndexID]; ok { + continue } + filteredExpectedZoneConfigSubzones = append( + filteredExpectedZoneConfigSubzones, + c, + ) } expectedZoneConfig.Subzones = filteredExpectedZoneConfigSubzones From 0fc4ab52d7ae53c92a4e28f8c772d7efcfe39cff Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Wed, 14 Apr 2021 07:07:20 +1000 Subject: [PATCH 5/6] sql: refactor SynthesizeRegionConfig to take in options Release note: None --- pkg/ccl/backupccl/restore_job.go | 8 +- pkg/sql/catalog/multiregion/region_config.go | 6 +- pkg/sql/region_util.go | 106 +++++++++---------- 3 files changed, 57 insertions(+), 63 deletions(-) diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index e222efa13843..6139689b30e8 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/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 } diff --git a/pkg/sql/catalog/multiregion/region_config.go b/pkg/sql/catalog/multiregion/region_config.go index 779c7a049566..107486312410 100644 --- a/pkg/sql/catalog/multiregion/region_config.go +++ b/pkg/sql/catalog/multiregion/region_config.go @@ -80,11 +80,9 @@ func (r *RegionConfig) RegionEnumID() descpb.ID { // MakeRegionConfigOption is an option for MakeRegionConfig type MakeRegionConfigOption func(r *RegionConfig) -// MakeRegionConfigOptionTransitioningRegions is an option to include transitioning +// WithTransitioningRegions is an option to include transitioning // regions into MakeRegionConfig. -func MakeRegionConfigOptionTransitioningRegions( - transitioningRegions descpb.RegionNames, -) MakeRegionConfigOption { +func WithTransitioningRegions(transitioningRegions descpb.RegionNames) MakeRegionConfigOption { return func(r *RegionConfig) { r.transitioningRegions = transitioningRegions } diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 906daa3ddd6f..fc3c2afd22de 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -835,7 +835,13 @@ func (p *planner) ValidateAllMultiRegionZoneConfigsInCurrentDatabase(ctx context if !dbDesc.IsMultiRegion() { return nil } - regionConfig, err := SynthesizeRegionConfigForZoneConfigValidation(ctx, p.txn, dbDesc.ID, p.Descriptors()) + regionConfig, err := SynthesizeRegionConfig( + ctx, + p.txn, + dbDesc.GetID(), + p.Descriptors(), + SynthesizeRegionConfigOptionForValidation, + ) if err != nil { return err } @@ -950,76 +956,48 @@ func (p *planner) CurrentDatabaseRegionConfig( ), nil } -// SynthesizeRegionConfigOffline is the public function for the synthesizing -// region configs in cases where the searched for descriptor may be in -// the offline state. See synthesizeRegionConfig for more details on what it -// does under the covers. -func SynthesizeRegionConfigOffline( - ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, -) (multiregion.RegionConfig, error) { - return synthesizeRegionConfigImpl( - ctx, - txn, - dbID, - descsCol, - true, /* includeOffline */ - false, /* forZoneConfigValidate */ - ) +type synthesizeRegionConfigOptions struct { + includeOffline bool + forValidation bool } -// SynthesizeRegionConfigForZoneConfigValidation returns a RegionConfig -// representing the user configured state of a multi-region database by -// coalescing state from both the database descriptor and multi-region type -// descriptor. It avoids the cache and is intended for use by DDL statements. -// Since it is intended to be called for validation of the RegionConfig against -// the current database zone configuration, it omits regions that are in the -// adding state, but includes those that are being dropped. -func SynthesizeRegionConfigForZoneConfigValidation( - ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, -) (multiregion.RegionConfig, error) { - return synthesizeRegionConfigImpl( - ctx, - txn, - dbID, - descsCol, - false, /* includeOffline */ - true, /* forZoneConfigValidate */ - ) +// SynthesizeRegionConfigOption is an option to pass into SynthesizeRegionConfig. +type SynthesizeRegionConfigOption func(o *synthesizeRegionConfigOptions) + +// SynthesizeRegionConfigOptionIncludeOffline includes offline descriptors for use +// in RESTORE. +var SynthesizeRegionConfigOptionIncludeOffline SynthesizeRegionConfigOption = func(o *synthesizeRegionConfigOptions) { + o.includeOffline = true } -// SynthesizeRegionConfig is the public function for the synthesizing region -// configs in the common case (i.e. not the offline case). See -// synthesizeRegionConfig for more details on what it does under the covers. -func SynthesizeRegionConfig( - ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, -) (multiregion.RegionConfig, error) { - return synthesizeRegionConfigImpl( - ctx, - txn, - dbID, - descsCol, - false, /* includeOffline */ - false, /* forZoneConfigValidate */ - ) +// SynthesizeRegionConfigOptionForValidation includes descriptors which are being dropped +// as part of the regions field, allowing validation to account for regions in the +// process of being dropped. +var SynthesizeRegionConfigOptionForValidation SynthesizeRegionConfigOption = func(o *synthesizeRegionConfigOptions) { + o.forValidation = true } -// SynthesizeRegionConfigImpl returns a RegionConfig representing the user +// SynthesizeRegionConfig returns a RegionConfig representing the user // configured state of a multi-region database by coalescing state from both // the database descriptor and multi-region type descriptor. It avoids the cache // and is intended for use by DDL statements. -func synthesizeRegionConfigImpl( +func SynthesizeRegionConfig( ctx context.Context, txn *kv.Txn, dbID descpb.ID, descsCol *descs.Collection, - includeOffline bool, - forZoneConfigValidate bool, + opts ...SynthesizeRegionConfigOption, ) (multiregion.RegionConfig, error) { + var o synthesizeRegionConfigOptions + for _, opt := range opts { + opt(&o) + } + regionConfig := multiregion.RegionConfig{} _, dbDesc, err := descsCol.GetImmutableDatabaseByID(ctx, txn, dbID, tree.DatabaseLookupFlags{ AvoidCached: true, Required: true, - IncludeOffline: includeOffline, + IncludeOffline: o.includeOffline, }) if err != nil { return regionConfig, err @@ -1037,7 +1015,7 @@ func synthesizeRegionConfigImpl( tree.ObjectLookupFlags{ CommonLookupFlags: tree.CommonLookupFlags{ AvoidCached: true, - IncludeOffline: includeOffline, + IncludeOffline: o.includeOffline, }, }, ) @@ -1046,7 +1024,7 @@ func synthesizeRegionConfigImpl( } var regionNames descpb.RegionNames - if forZoneConfigValidate { + if o.forValidation { regionNames, err = regionEnum.RegionNamesForValidation() } else { regionNames, err = regionEnum.RegionNames() @@ -1065,7 +1043,7 @@ func synthesizeRegionConfigImpl( dbDesc.RegionConfig.PrimaryRegion, dbDesc.RegionConfig.SurvivalGoal, regionEnumID, - multiregion.MakeRegionConfigOptionTransitioningRegions(transitioningRegionNames), + multiregion.WithTransitioningRegions(transitioningRegionNames), ) if err := multiregion.ValidateRegionConfig(regionConfig); err != nil { @@ -1391,7 +1369,13 @@ func (p *planner) validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser( if err != nil { return err } - regionConfig, err := SynthesizeRegionConfigForZoneConfigValidation(ctx, p.txn, dbDesc.ID, p.Descriptors()) + regionConfig, err := SynthesizeRegionConfig( + ctx, + p.txn, + dbDesc.GetID(), + p.Descriptors(), + SynthesizeRegionConfigOptionForValidation, + ) if err != nil { return err } @@ -1458,7 +1442,13 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( if err != nil { return err } - regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, dbDesc.GetID(), p.Descriptors()) + regionConfig, err := SynthesizeRegionConfig( + ctx, + p.txn, + dbDesc.GetID(), + p.Descriptors(), + SynthesizeRegionConfigOptionForValidation, + ) if err != nil { return err } From ab264657df0ee7537ca895f26202377e285e56bf Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 16 Apr 2021 06:39:22 +1000 Subject: [PATCH 6/6] sql: refactor SynthesizeRegionConfig usages Use the option method instead to decide to read from the cache. Release note: None --- pkg/sql/region_util.go | 44 +++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index fc3c2afd22de..b993b09d62b0 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -907,7 +907,7 @@ func (p *planner) validateAllMultiRegionZoneConfigsInDatabase( // CurrentDatabaseRegionConfig is part of the tree.EvalDatabase interface. // CurrentDatabaseRegionConfig uses the cache to synthesize the RegionConfig -// and as such is intended for DML use. It returns an empty DatabaseRegionConfig +// and as such is intended for DML use. It returns nil // if the current database is not multi-region enabled. func (p *planner) CurrentDatabaseRegionConfig( ctx context.Context, @@ -928,37 +928,19 @@ func (p *planner) CurrentDatabaseRegionConfig( return nil, nil } - // Construct a region config from leased descriptors. - regionEnumID, err := dbDesc.MultiRegionEnumID() - if err != nil { - return nil, err - } - - regionEnum, err := p.Descriptors().GetImmutableTypeByID( + return SynthesizeRegionConfig( ctx, p.txn, - regionEnumID, - tree.ObjectLookupFlags{}, + dbDesc.GetID(), + p.Descriptors(), + SynthesizeRegionConfigOptionUseCache, ) - if err != nil { - return nil, err - } - regionNames, err := regionEnum.RegionNames() - if err != nil { - return nil, err - } - - return multiregion.MakeRegionConfig( - regionNames, - dbDesc.RegionConfig.PrimaryRegion, - dbDesc.RegionConfig.SurvivalGoal, - regionEnumID, - ), nil } type synthesizeRegionConfigOptions struct { includeOffline bool forValidation bool + useCache bool } // SynthesizeRegionConfigOption is an option to pass into SynthesizeRegionConfig. @@ -977,10 +959,16 @@ var SynthesizeRegionConfigOptionForValidation SynthesizeRegionConfigOption = fun o.forValidation = true } +// SynthesizeRegionConfigOptionUseCache uses a cache for synthesizing the region +// config. +var SynthesizeRegionConfigOptionUseCache SynthesizeRegionConfigOption = func(o *synthesizeRegionConfigOptions) { + o.useCache = true +} + // SynthesizeRegionConfig returns a RegionConfig representing the user // configured state of a multi-region database by coalescing state from both -// the database descriptor and multi-region type descriptor. It avoids the cache -// and is intended for use by DDL statements. +// the database descriptor and multi-region type descriptor. By default, it +// avoids the cache and is intended for use by DDL statements. func SynthesizeRegionConfig( ctx context.Context, txn *kv.Txn, @@ -995,7 +983,7 @@ func SynthesizeRegionConfig( regionConfig := multiregion.RegionConfig{} _, dbDesc, err := descsCol.GetImmutableDatabaseByID(ctx, txn, dbID, tree.DatabaseLookupFlags{ - AvoidCached: true, + AvoidCached: !o.useCache, Required: true, IncludeOffline: o.includeOffline, }) @@ -1014,7 +1002,7 @@ func SynthesizeRegionConfig( regionEnumID, tree.ObjectLookupFlags{ CommonLookupFlags: tree.CommonLookupFlags{ - AvoidCached: true, + AvoidCached: !o.useCache, IncludeOffline: o.includeOffline, }, },