From 9471fb1560c80471e146ff3995ab80f6343e98ac Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Wed, 31 Mar 2021 13:13:08 +1100 Subject: [PATCH 1/2] sql: add crdb_internal.validate_multi_region_zone_configs builtin In addition to the release note, added some pgcodes to validation errors as well as minor refactoring to make it work and error nicely with the "overriding" error message. Also fix up some of the string escaping to use tree.Name. Release note (sql change): Add a new builtin `crdb_internal.validate_multi_region_zone_configs` which validates all zone configurations in the current database are correctly configured. --- docs/generated/sql/functions.md | 15 +- pkg/ccl/importccl/import_table_creation.go | 7 + .../logic_test/multi_region_zone_configs | 30 ++- pkg/sql/alter_table_locality.go | 6 +- pkg/sql/faketreeeval/evalctx.go | 14 ++ pkg/sql/region_util.go | 210 +++++++++++++----- pkg/sql/sem/builtins/builtins.go | 22 +- pkg/sql/sem/tree/eval.go | 5 + 8 files changed, 243 insertions(+), 66 deletions(-) diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md index a7c0e8287ef7..85534481846b 100644 --- a/docs/generated/sql/functions.md +++ b/docs/generated/sql/functions.md @@ -873,6 +873,16 @@ has no relationship with the commit order of concurrent transactions.

+ + -
Function → ReturnsDescription
crdb_internal.filter_multiregion_fields_from_zone_config_sql(val: string) → string

Takes in a CONFIGURE ZONE SQL statement and returns a modified +SQL statement omitting multi-region related zone configuration fields. +If the CONFIGURE ZONE statement can be inferred by the database’s or +table’s zone configuration this will return NULL.

+
crdb_internal.validate_multi_region_zone_configs() → bool

Validates all multi-region zone configurations are correctly setup +for the current database, including all tables, indexes and partitions underneath. +Returns an error if validation fails. This builtin uses un-leased versions of the +each descriptor, requiring extra round trips.

+
default_to_database_primary_region(val: string) → string

Returns the given region if the region has been added to the current database. Otherwise, this will return the primary region of the current database. This will error if the current database is not a multi-region database.

@@ -2372,11 +2382,6 @@ The swap_ordinate_string parameter is a 2-character string naming the ordinates
convert_to(str: string, enc: string) → bytes

Encode the string str as a byte array using encoding enc. Supports encodings ‘UTF8’ and ‘LATIN1’.

crdb_internal.filter_multiregion_fields_from_zone_config_sql(val: string) → string

Takes in a CONFIGURE ZONE SQL statement and returns a modified -SQL statement omitting multi-region related zone configuration fields. -If the CONFIGURE ZONE statement can be inferred by the database’s or -table’s zone configuration this will return NULL.

-
crdb_internal.show_create_all_tables(database_name: string) → string

Returns rows of CREATE table statements followed by ALTER table statements that add table constraints. The rows are ordered by dependencies. All foreign keys are added after the creation of the table diff --git a/pkg/ccl/importccl/import_table_creation.go b/pkg/ccl/importccl/import_table_creation.go index 0f30aaff17cd..cefc1183adeb 100644 --- a/pkg/ccl/importccl/import_table_creation.go +++ b/pkg/ccl/importccl/import_table_creation.go @@ -223,6 +223,13 @@ func (so *importSequenceOperators) CurrentDatabaseRegionConfig( return nil, errors.WithStack(errSequenceOperators) } +// ValidateAllMultiRegionZoneConfigsInCurrentDatabase is part of the tree.EvalDatabase interface. +func (so *importSequenceOperators) ValidateAllMultiRegionZoneConfigsInCurrentDatabase( + _ context.Context, +) error { + return errors.WithStack(errSequenceOperators) +} + // Implements the tree.EvalDatabase interface. func (so *importSequenceOperators) ParseQualifiedTableName(sql string) (*tree.TableName, error) { name, err := parser.ParseTableName(sql) 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 d1106e68085b..dfdb6878c5b6 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs @@ -7,12 +7,18 @@ ap-southeast-2 {ap-az1,ap-az2,ap-az3} {} {} ca-central-1 {ca-az1,ca-az2,ca-az3} {} {} us-east-1 {us-az1,us-az2,us-az3} {} {} +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + statement ok CREATE DATABASE "mr-zone-configs" primary region "ca-central-1" regions "ap-southeast-2", "us-east-1" statement ok use "mr-zone-configs" +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + statement ok ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING gc.ttlseconds = 5 @@ -32,6 +38,9 @@ DATABASE "mr-zone-configs" ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USIN voter_constraints = '[+region=ca-central-1]', lease_preferences = '[[+region=ca-central-1]]' +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + statement error attempting to modify protected field "num_voters" of a multi-region zone configuration ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING num_voters = 5 @@ -53,6 +62,9 @@ SET override_multi_region_zone_config = true; ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING num_voters = 5; SET override_multi_region_zone_config = false +statement error zone configuration for database "mr-zone-configs" contains incorrectly configured field "num_voters" +SELECT crdb_internal.validate_multi_region_zone_configs() + query TT SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-configs" ---- @@ -308,6 +320,9 @@ CREATE TABLE regional_by_table ( statement ok ALTER table regional_by_row CONFIGURE ZONE USING gc.ttlseconds = 10 +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + statement error attempting to modify protected field "num_replicas" of a multi-region zone configuration ALTER table regional_by_row CONFIGURE ZONE USING num_replicas = 10 @@ -316,6 +331,9 @@ SET override_multi_region_zone_config = true; ALTER table regional_by_row CONFIGURE ZONE USING num_replicas = 10; SET override_multi_region_zone_config = false +statement error zone configuration for table regional_by_row contains incorrectly configured field "num_replicas" +SELECT crdb_internal.validate_multi_region_zone_configs() + query TT SHOW ZONE CONFIGURATION FOR TABLE regional_by_row ---- @@ -361,7 +379,7 @@ num_voters = 3, voter_constraints = '[+region=us-east-1]', lease_preferences = '[[+region=us-east-1]]' regional_by_row@regional_by_row_i_idx us-east-1 -statement error attempting to update zone configuration for table "regional_by_row" which contains modified field "num_replicas" +statement error attempting to update zone configuration for table regional_by_row which contains modified field "num_replicas" ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY TABLE statement ok @@ -422,6 +440,9 @@ ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY ROW statement ok ALTER TABLE regional_by_row SET LOCALITY GLOBAL +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + statement ok SET override_multi_region_zone_config = true; ALTER index regional_by_row@primary CONFIGURE ZONE USING num_replicas = 10; @@ -430,7 +451,10 @@ SET override_multi_region_zone_config = false statement ok ALTER TABLE regional_by_row SET LOCALITY GLOBAL -statement error attempting to update zone configuration for table "regional_by_row" which contains a zone configuration on index "primary" +statement error zone configuration for index regional_by_row@"primary" contains incorrectly configured field "num_replicas" +SELECT crdb_internal.validate_multi_region_zone_configs() + +statement error attempting to update zone configuration for index regional_by_row@"primary" which contains modified field "num_replicas" ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY ROW statement ok @@ -465,7 +489,7 @@ INDEX regional_by_row_as@primary ALTER INDEX regional_by_row_as@primary CONFIGU voter_constraints = '[+region=us-east-1]', lease_preferences = '[[+region=us-east-1]]' -statement error attempting to update zone configuration for table "regional_by_row_as" which contains a zone configuration on index "primary" with multi-region field "num_replicas" set +statement error attempting to update zone configuration for index regional_by_row_as@"primary" which contains modified field "num_replicas" ALTER TABLE regional_by_row_as SET LOCALITY REGIONAL BY ROW statement ok diff --git a/pkg/sql/alter_table_locality.go b/pkg/sql/alter_table_locality.go index bb194cdd6330..7ae2242af8f1 100644 --- a/pkg/sql/alter_table_locality.go +++ b/pkg/sql/alter_table_locality.go @@ -470,13 +470,13 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error { ), ) - toRegionalByRow := newLocality.LocalityLevel == tree.LocalityLevelRow + // 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, - toRegionalByRow, - ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes, + checkIndexZoneConfigs, ); err != nil { return err } diff --git a/pkg/sql/faketreeeval/evalctx.go b/pkg/sql/faketreeeval/evalctx.go index a302269f6065..3fd511b017e8 100644 --- a/pkg/sql/faketreeeval/evalctx.go +++ b/pkg/sql/faketreeeval/evalctx.go @@ -51,6 +51,13 @@ func (so *DummySequenceOperators) CurrentDatabaseRegionConfig( return nil, errors.WithStack(errSequenceOperators) } +// ValidateAllMultiRegionZoneConfigsInCurrentDatabase is part of the tree.EvalDatabase interface. +func (so *DummySequenceOperators) ValidateAllMultiRegionZoneConfigsInCurrentDatabase( + _ context.Context, +) error { + return errors.WithStack(errSequenceOperators) +} + // ParseQualifiedTableName is part of the tree.EvalDatabase interface. func (so *DummySequenceOperators) ParseQualifiedTableName(sql string) (*tree.TableName, error) { return nil, errors.WithStack(errSequenceOperators) @@ -186,6 +193,13 @@ func (ep *DummyEvalPlanner) CurrentDatabaseRegionConfig( return nil, errors.WithStack(errEvalPlanner) } +// ValidateAllMultiRegionZoneConfigsInCurrentDatabase is part of the tree.EvalDatabase interface. +func (ep *DummyEvalPlanner) ValidateAllMultiRegionZoneConfigsInCurrentDatabase( + _ context.Context, +) error { + return errors.WithStack(errEvalPlanner) +} + // ParseQualifiedTableName is part of the tree.EvalDatabase interface. func (ep *DummyEvalPlanner) ParseQualifiedTableName(sql string) (*tree.TableName, error) { return parser.ParseQualifiedTableName(sql) diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 45f71d6e9ce0..547c3c5ae9d4 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -14,7 +14,6 @@ import ( "context" "fmt" "sort" - "strconv" "strings" "github.com/cockroachdb/cockroach/pkg/config/zonepb" @@ -832,6 +831,45 @@ func partitionByForRegionalByRow( } } +// ValidateAllMultiRegionZoneConfigsInCurrentDatabase is part of the tree.EvalDatabase interface. +func (p *planner) ValidateAllMultiRegionZoneConfigsInCurrentDatabase(ctx context.Context) error { + _, dbDesc, err := p.Descriptors().GetImmutableDatabaseByName( + p.EvalContext().Ctx(), + p.txn, + p.CurrentDatabase(), + tree.DatabaseLookupFlags{ + Required: true, + }, + ) + if err != nil { + return err + } + if !dbDesc.IsMultiRegion() { + return nil + } + + if err := p.validateZoneConfigForMultiRegionDatabase( + ctx, + dbDesc, + &validateZoneConfigForMultiRegionErrorHandlerValidation{}, + ); err != nil { + return err + } + return p.forEachTableInMultiRegionDatabase( + ctx, + dbDesc, + func(ctx context.Context, tbDesc *tabledesc.Mutable) error { + return p.validateZoneConfigForMultiRegionTable( + ctx, + dbDesc, + tbDesc, + true, /* checkIndexZoneConfig */ + &validateZoneConfigForMultiRegionErrorHandlerValidation{}, + ) + }, + ) +} + // 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 @@ -843,7 +881,9 @@ func (p *planner) CurrentDatabaseRegionConfig( p.EvalContext().Ctx(), p.txn, p.CurrentDatabase(), - tree.DatabaseLookupFlags{Required: true}, + tree.DatabaseLookupFlags{ + Required: true, + }, ) if err != nil { return nil, err @@ -1076,6 +1116,54 @@ func (p *planner) CheckZoneConfigChangePermittedForMultiRegion( return nil } +// validateZoneConfigForMultiRegionErrorHandler is an interface representing +// an error to generate if validating a zone config for multi-region +// fails. +type validateZoneConfigForMultiRegionErrorHandler interface { + newError(descType string, descName string, field string) error +} + +// validateZoneConfigForMultiRegionErrorHandlerModifiedByUser implements +// interface validateZoneConfigForMultiRegionErrorHandler. +type validateZoneConfigForMultiRegionErrorHandlerModifiedByUser struct{} + +func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) newError( + descType string, descName string, field string, +) error { + err := pgerror.Newf( + pgcode.InvalidObjectDefinition, + "attempting to update zone configuration for %s %s which contains modified field %q", + descType, + descName, + field, + ) + err = errors.WithDetail( + err, + "the attempted operation will overwrite a user modified field", + ) + return errors.WithHint( + err, + "to proceed with the overwrite, SET override_multi_region_zone_config = true, "+ + "and reissue the statement", + ) +} + +// validateZoneConfigForMultiRegionErrorHandlerValidation implements +// interface validateZoneConfigForMultiRegionErrorHandler. +type validateZoneConfigForMultiRegionErrorHandlerValidation struct{} + +func (v *validateZoneConfigForMultiRegionErrorHandlerValidation) newError( + descType string, descName string, field string, +) error { + return pgerror.Newf( + pgcode.InvalidObjectDefinition, + "zone configuration for %s %s contains incorrectly configured field %q", + descType, + descName, + field, + ) +} + // validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser validates that // the zone configuration was not modified by the user. The function is intended // to be called in cases where a multi-region operation will overwrite the @@ -1088,7 +1176,20 @@ func (p *planner) validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser( if p.SessionData().OverrideMultiRegionZoneConfigEnabled { return nil } + return p.validateZoneConfigForMultiRegionDatabase( + ctx, + dbDesc, + &validateZoneConfigForMultiRegionErrorHandlerModifiedByUser{}, + ) +} +// validateZoneConfigForMultiRegionDatabase validates that the zone config +// for the databases matches as the multi-region database definition. +func (p *planner) validateZoneConfigForMultiRegionDatabase( + ctx context.Context, + dbDesc *dbdesc.Immutable, + validateZoneConfigForMultiRegionErrorHandler validateZoneConfigForMultiRegionErrorHandler, +) error { regionConfig, err := SynthesizeRegionConfigForZoneConfigValidation(ctx, p.txn, dbDesc.ID, p.Descriptors()) if err != nil { return err @@ -1108,14 +1209,12 @@ func (p *planner) validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser( return err } if !same { - err := errors.Newf( - "attempting to update zone configuration for database %q which contains modified field %q ", - dbDesc.GetName(), + dbName := tree.Name(dbDesc.GetName()) + return validateZoneConfigForMultiRegionErrorHandler.newError( + "database", + dbName.String(), field, ) - err = errors.WithDetail(err, "the attempted operation will overwrite "+ - "a user modified field") - return errors.WithHint(err, "to proceed with the override, SET override_multi_region_zone_config = true, and reissue the statement") } return nil @@ -1131,8 +1230,7 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( ctx context.Context, dbDesc *dbdesc.Immutable, desc *tabledesc.Mutable, - toRegionalByRow bool, - opts ...applyZoneConfigForMultiRegionTableOption, + checkIndexZoneConfigs bool, ) error { // If the user is overriding, or this is not a multi-region table our work here // is done. @@ -1140,6 +1238,24 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( return nil } + return p.validateZoneConfigForMultiRegionTable( + ctx, + dbDesc, + desc, + checkIndexZoneConfigs, + &validateZoneConfigForMultiRegionErrorHandlerModifiedByUser{}, + ) +} + +// validateZoneConfigForMultiRegionTableOptions validates that +// the table's zone configuration matches exactly what is expected. +func (p *planner) validateZoneConfigForMultiRegionTable( + ctx context.Context, + dbDesc *dbdesc.Immutable, + desc catalog.TableDescriptor, + checkIndexZoneConfigs bool, + validateZoneConfigForMultiRegionErrorHandler validateZoneConfigForMultiRegionErrorHandler, +) error { currentZoneConfig, err := getZoneConfigRaw(ctx, p.txn, p.ExecCfg().Codec, desc.GetID()) if err != nil { return err @@ -1148,14 +1264,19 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( currentZoneConfig = zonepb.NewZoneConfig() } - // The expected zone config starts from the same base config as the current - // zone config, so copy it over to be used down below. - expectedZoneConfig := currentZoneConfig + regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, dbDesc.ID, p.Descriptors()) + if err != nil { + return err + } - if toRegionalByRow { - // We're going to REGIONAL BY ROW. Check to see if the to be applied zone - // configurations will override any existing zone configurations on the - // table's indexes. We say "override" here, and not "overwrite" because + tableName := tree.Name(desc.GetName()) + + // TODO(#62790): we should check partition zone configs match on the + // validate zone config case. + if checkIndexZoneConfigs { + // Check to see if the to be applied zone configurations will override + // any existing zone configurations on the table's indexes. + // We say "override" here, and not "overwrite" because // REGIONAL BY ROW tables will not write zone configs at the index level, // but instead, at the index partition level. That being said, application // of a partition-level zone config will override any applied index-level @@ -1164,75 +1285,56 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( if s.PartitionName == "" { // Found a zone config on an index. Check to see if any of its // multi-region fields are set. - if isSet, str := s.Config.IsAnyMultiRegionFieldSet(); isSet { + if isSet, field := s.Config.IsAnyMultiRegionFieldSet(); isSet { // Find the name of the offending index to use in the message below. // In the case where we can't find the name, do our best and return // the ID. - indexName := fmt.Sprintf("unknown with ID = %s", - strconv.FormatUint(uint64(s.IndexID), 10)) + indexName := fmt.Sprintf("[%d]", s.IndexID) for _, i := range desc.ActiveIndexes() { if uint32(i.GetID()) == s.IndexID { - indexName = i.GetName() + indexTreeName := tree.Name(i.GetName()) + indexName = indexTreeName.String() } } - err := errors.Newf( - "attempting to update zone configuration for table %q which "+ - "contains a zone configuration on index %q with multi-region field %q set", - desc.GetName(), - indexName, - str, + return validateZoneConfigForMultiRegionErrorHandler.newError( + "index", + fmt.Sprintf("%s@%s", tableName.String(), indexName), + field, ) - err = errors.WithDetail(err, "the attempted operation will override "+ - "the index zone configuration field") - return errors.WithHint(err, "to proceed with the override, SET "+ - "override_multi_region_zone_config = true, and reissue the statement") } } } } - regionConfig, err := SynthesizeRegionConfig(ctx, p.txn, dbDesc.ID, p.Descriptors()) + _, expectedZoneConfig, err := ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes( + *currentZoneConfig, + regionConfig, + desc, + ) if err != nil { return err } - // Fill in the expectedZoneConfig using the specified option. - for _, opt := range opts { - _, newZoneConfig, err := opt( - *expectedZoneConfig, - regionConfig, - desc, - ) - if err != nil { - return err - } - expectedZoneConfig = &newZoneConfig - } - // Mark the NumReplicas as 0 if we have subzones but no other features // in the zone config. This signifies a placeholder. - if len(expectedZoneConfig.Subzones) > 0 && isPlaceholderZoneConfigForMultiRegion(*expectedZoneConfig) { + if len(expectedZoneConfig.Subzones) > 0 && isPlaceholderZoneConfigForMultiRegion(expectedZoneConfig) { expectedZoneConfig.NumReplicas = proto.Int32(0) } // Compare the two zone configs to see if anything is amiss. same, field, err := currentZoneConfig.DiffWithZone( - *expectedZoneConfig, + expectedZoneConfig, zonepb.MultiRegionZoneConfigFields, ) if err != nil { return err } if !same { - err := errors.Newf( - "attempting to update zone configuration for table %q which contains modified field %q ", - desc.GetName(), + return validateZoneConfigForMultiRegionErrorHandler.newError( + "table", + tableName.String(), field, ) - err = errors.WithDetail(err, "the attempted operation will overwrite "+ - "a user modified field") - return errors.WithHint(err, "to proceed with the overwrite, SET "+ - "override_multi_region_zone_config = true, and reissue the statement") } return nil diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index bb962644bb7d..3f37a54a8620 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -5027,8 +5027,28 @@ the locality flag on node startup. Returns an error if no region is set.`, tree.VolatilityStable, ), ), + "crdb_internal.validate_multi_region_zone_configs": makeBuiltin( + tree.FunctionProperties{Category: categoryMultiRegion}, + tree.Overload{ + Types: tree.ArgTypes{}, + ReturnType: tree.FixedReturnType(types.Bool), + Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + if err := evalCtx.Sequence.ValidateAllMultiRegionZoneConfigsInCurrentDatabase( + evalCtx.Context, + ); err != nil { + return nil, err + } + return tree.MakeDBool(true), nil + }, + Info: `Validates all multi-region zone configurations are correctly setup + for the current database, including all tables, indexes and partitions underneath. + Returns an error if validation fails. This builtin uses un-leased versions of the + each descriptor, requiring extra round trips.`, + Volatility: tree.VolatilityVolatile, + }, + ), "crdb_internal.filter_multiregion_fields_from_zone_config_sql": makeBuiltin( - tree.FunctionProperties{}, + tree.FunctionProperties{Category: categoryMultiRegion}, stringOverload1( func(evalCtx *tree.EvalContext, s string) (tree.Datum, error) { stmt, err := parser.ParseOne(s) diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 89817ede4383..b234dbd1fcc4 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -3017,6 +3017,11 @@ type EvalDatabase interface { // session database. CurrentDatabaseRegionConfig(ctx context.Context) (DatabaseRegionConfig, error) + // ValidateAllMultiRegionZoneConfigsInCurrentDatabase validates whether the current + // database's multi-region zone configs are correctly setup. This includes + // all tables within the database. + ValidateAllMultiRegionZoneConfigsInCurrentDatabase(ctx context.Context) error + // ParseQualifiedTableName parses a SQL string of the form // `[ database_name . ] [ schema_name . ] table_name`. // NB: this is deprecated! Use parser.ParseQualifiedTableName when possible. From 5f5101b0ac9e15aa9bf86259353cf350adfd8c6f Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 1 Apr 2021 06:50:18 +1100 Subject: [PATCH 2/2] zonepb: make subzone DiffWithZone more accurate * Subzones may be defined in a different order. We did not take this into account which can cause bugs when e.g. ADD REGION adds a subzone in the end rather than in the old "expected" location in the subzones array. This has been fixed by comparing subzones using an unordered map. * The ApplyZoneConfig we previously did overwrote subzone fields on the original subzone array element, meaning that if there was a mismatch it would not be reported through validation. This is now fixed by applying the expected zone config to *zonepb.NewZoneConfig() instead. * Added logic to only check for zone config matches subzones from active subzone IDs. * Improve the error messaging when a subzone config is mismatching - namely, add index and partitioning information and differentiate between missing fields and missing / extraneous zone configs Release note (bug fix): Fixed validation bugs during ALTER TABLE ... SET LOCALITY / crdb_internal.validate_multi_region_zone_config where validation errors could occur when the database of a REGIONAL BY ROW table has a new region added. Also fix a validation bug partition zone mismatches configs were not caught. --- .../logic_test/multi_region_zone_configs | 46 +++- .../testdata/logic_test/regional_by_row | 3 + pkg/config/zonepb/zone.go | 177 ++++++++++----- pkg/sql/region_util.go | 211 +++++++++++++----- 4 files changed, 322 insertions(+), 115 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 dfdb6878c5b6..bbf0f73406b8 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs +++ b/pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_configs @@ -435,11 +435,47 @@ TABLE regional_by_row ALTER TABLE regional_by_row CONFIGURE ZONE USING lease_preferences = '[[+region=us-east-1]]' statement ok -ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY ROW +DROP TABLE regional_by_row; CREATE TABLE regional_by_row ( + pk INT PRIMARY KEY, + i INT, + INDEX idx(i), + FAMILY (pk, i) +) LOCALITY REGIONAL BY ROW + +statement ok +ALTER PARTITION "ap-southeast-2" OF INDEX regional_by_row@idx CONFIGURE ZONE USING gc.ttlseconds = 10; +ALTER INDEX regional_by_row@idx CONFIGURE ZONE USING gc.ttlseconds = 10 + +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + +statement ok +SET override_multi_region_zone_config = true; +ALTER PARTITION "ap-southeast-2" OF INDEX regional_by_row@idx CONFIGURE ZONE USING num_replicas = 10; +SET override_multi_region_zone_config = false + +statement error zone configuration for partition "ap-southeast-2" of regional_by_row@idx contains incorrectly configured field "num_replicas" +SELECT crdb_internal.validate_multi_region_zone_configs() + +statement error attempting to update zone configuration for partition "ap-southeast-2" of regional_by_row@idx which contains modified field "num_replicas" +ALTER TABLE regional_by_row SET LOCALITY GLOBAL statement ok +SET override_multi_region_zone_config = true; +ALTER PARTITION "ap-southeast-2" OF INDEX regional_by_row@idx CONFIGURE ZONE DISCARD; +SET override_multi_region_zone_config = false + +statement error missing zone configuration for partition "ap-southeast-2" of regional_by_row@idx +SELECT crdb_internal.validate_multi_region_zone_configs() + +statement error attempting to update zone config which is missing an expected zone configuration for partition "ap-southeast-2" of regional_by_row@idx ALTER TABLE regional_by_row SET LOCALITY GLOBAL +statement ok +SET override_multi_region_zone_config = true; +ALTER TABLE regional_by_row SET LOCALITY GLOBAL; +SET override_multi_region_zone_config = false + statement ok SELECT crdb_internal.validate_multi_region_zone_configs() @@ -448,13 +484,13 @@ SET override_multi_region_zone_config = true; ALTER index regional_by_row@primary CONFIGURE ZONE USING num_replicas = 10; SET override_multi_region_zone_config = false -statement ok +statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row@"primary" ALTER TABLE regional_by_row SET LOCALITY GLOBAL -statement error zone configuration for index regional_by_row@"primary" contains incorrectly configured field "num_replicas" +statement error extraneous zone configuration for index regional_by_row@"primary" SELECT crdb_internal.validate_multi_region_zone_configs() -statement error attempting to update zone configuration for index regional_by_row@"primary" which contains modified field "num_replicas" +statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row@"primary" ALTER TABLE regional_by_row SET LOCALITY REGIONAL BY ROW statement ok @@ -489,7 +525,7 @@ INDEX regional_by_row_as@primary ALTER INDEX regional_by_row_as@primary CONFIGU voter_constraints = '[+region=us-east-1]', lease_preferences = '[[+region=us-east-1]]' -statement error attempting to update zone configuration for index regional_by_row_as@"primary" which contains modified field "num_replicas" +statement error attempting to update zone config which contains an extra zone configuration for index regional_by_row_as@"primary" ALTER TABLE regional_by_row_as SET LOCALITY REGIONAL BY ROW statement ok diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index ed23b6dbf1f3..e81a2eb32fad 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -1601,6 +1601,9 @@ DATABASE add_regions ALTER DATABASE add_regions CONFIGURE ZONE USING statement ok ALTER DATABASE add_regions ADD REGION "us-east-1" +statement ok +SELECT crdb_internal.validate_multi_region_zone_configs() + query TT SHOW CREATE TABLE regional_by_row ---- diff --git a/pkg/config/zonepb/zone.go b/pkg/config/zonepb/zone.go index e7adf6f86c72..670c0c10f8b5 100644 --- a/pkg/config/zonepb/zone.go +++ b/pkg/config/zonepb/zone.go @@ -78,30 +78,6 @@ var MultiRegionZoneConfigFieldsSet = func() map[tree.Name]struct{} { return ret }() -// IsAnyMultiRegionFieldSet returns true if any of the multi-region fields are -// set on the given zone config, and false if none of them are set. -func (z *ZoneConfig) IsAnyMultiRegionFieldSet() (bool, string) { - if z.GlobalReads != nil { - return true, "global_reads" - } - if z.NumVoters != nil { - return true, "num_voters" - } - if z.NumReplicas != nil { - return true, "num_replicas" - } - if len(z.Constraints) != 0 { - return true, "constraints" - } - if len(z.LeasePreferences) != 0 { - return true, "lease_preferences" - } - if len(z.VoterConstraints) != 0 { - return true, "voter_constraints" - } - return false, "" -} - // ZoneSpecifierFromID creates a tree.ZoneSpecifier for the zone with the // given ID. func ZoneSpecifierFromID( @@ -625,10 +601,33 @@ func (z *ZoneConfig) CopyFromZone(other ZoneConfig, fieldList []tree.Name) { } } +// DiffWithZoneMismatch indicates a mismatch between zone configurations. +type DiffWithZoneMismatch struct { + // NOTE: the below fields are only set if there is a subzone in the + // zone configuration which is mismatching. + + // IndexID represents a subzone with a mismatching index ID. + IndexID uint32 + // PartitionName represents a subzone with a mismatching partitionName. + PartitionName string + + // NOTE: only one of the below fields is set. + + // IsMissingSubzone indicates a subzone is missing. + IsMissingSubzone bool + // IsExtraSubzone indicates we have an extraneous subzone. + IsExtraSubzone bool + // Field indicates the field which is wrong. + Field string +} + // DiffWithZone diffs all specified fields of the supplied ZoneConfig, with the // receiver ZoneConfig. Returns true if all are equal, and false if there is a -// difference (along with a string which represents the first difference found). -func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool, string, error) { +// difference (along with a DiffWithZoneMismatch which represents the first +// difference found). +func (z *ZoneConfig) DiffWithZone( + other ZoneConfig, fieldList []tree.Name, +) (bool, DiffWithZoneMismatch, error) { for _, fieldName := range fieldList { switch fieldName { case "num_replicas": @@ -637,7 +636,9 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool } if z.NumReplicas == nil || other.NumReplicas == nil || *z.NumReplicas != *other.NumReplicas { - return false, "num_replicas", nil + return false, DiffWithZoneMismatch{ + Field: "num_replicas", + }, nil } case "num_voters": if other.NumVoters == nil && z.NumVoters == nil { @@ -645,7 +646,9 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool } if z.NumVoters == nil || other.NumVoters == nil || *z.NumVoters != *other.NumVoters { - return false, "num_voters", nil + return false, DiffWithZoneMismatch{ + Field: "num_voters", + }, nil } case "range_min_bytes": if other.RangeMinBytes == nil && z.RangeMinBytes == nil { @@ -653,7 +656,9 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool } if z.RangeMinBytes == nil || other.RangeMinBytes == nil || *z.RangeMinBytes != *other.RangeMinBytes { - return false, "range_min_bytes", nil + return false, DiffWithZoneMismatch{ + Field: "range_min_bytes", + }, nil } case "range_max_bytes": if other.RangeMaxBytes == nil && z.RangeMaxBytes == nil { @@ -661,7 +666,9 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool } if z.RangeMaxBytes == nil || other.RangeMaxBytes == nil || *z.RangeMaxBytes != *other.RangeMaxBytes { - return false, "range_max_bytes", nil + return false, DiffWithZoneMismatch{ + Field: "range_max_bytes", + }, nil } case "global_reads": if other.GlobalReads == nil && z.GlobalReads == nil { @@ -669,28 +676,36 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool } if z.GlobalReads == nil || other.GlobalReads == nil || *z.GlobalReads != *other.GlobalReads { - return false, "global_reads", nil + return false, DiffWithZoneMismatch{ + Field: "global_reads", + }, nil } case "gc.ttlseconds": if other.GC == nil && z.GC == nil { continue } if z.GC == nil || other.GC == nil || *z.GC != *other.GC { - return false, "gc.ttlseconds", nil + return false, DiffWithZoneMismatch{ + Field: "gc.ttlseconds", + }, nil } case "constraints": if other.Constraints == nil && z.Constraints == nil { continue } if z.Constraints == nil || other.Constraints == nil { - return false, "constraints", nil + return false, DiffWithZoneMismatch{ + Field: "constraints", + }, nil } for i, c := range z.Constraints { for j, constraint := range c.Constraints { if len(other.Constraints) <= i || len(other.Constraints[i].Constraints) <= j || constraint != other.Constraints[i].Constraints[j] { - return false, "constraints", nil + return false, DiffWithZoneMismatch{ + Field: "constraints", + }, nil } } } @@ -699,14 +714,18 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool continue } if z.VoterConstraints == nil || other.VoterConstraints == nil { - return false, "voter_constraints", nil + return false, DiffWithZoneMismatch{ + Field: "voter_constraints", + }, nil } for i, c := range z.VoterConstraints { for j, constraint := range c.Constraints { if len(other.VoterConstraints) <= i || len(other.VoterConstraints[i].Constraints) <= j || constraint != other.VoterConstraints[i].Constraints[j] { - return false, "voter_constraints", nil + return false, DiffWithZoneMismatch{ + Field: "voter_constraints", + }, nil } } } @@ -715,40 +734,98 @@ func (z *ZoneConfig) DiffWithZone(other ZoneConfig, fieldList []tree.Name) (bool continue } if z.LeasePreferences == nil || other.LeasePreferences == nil { - return false, "voter_constraints", nil + return false, DiffWithZoneMismatch{ + Field: "voter_constraints", + }, nil } for i, c := range z.LeasePreferences { for j, constraint := range c.Constraints { if len(other.LeasePreferences) <= i || len(other.LeasePreferences[i].Constraints) <= j || constraint != other.LeasePreferences[i].Constraints[j] { - return false, "lease_preferences", nil + return false, DiffWithZoneMismatch{ + Field: "lease_preferences", + }, nil } } } default: - return false, "", errors.AssertionFailedf("unknown zone configuration field %q", fieldName) + return false, DiffWithZoneMismatch{}, errors.AssertionFailedf("unknown zone configuration field %q", fieldName) } } // Look into all subzones and ensure they're equal across both zone // configs. - if len(z.Subzones) != len(other.Subzones) { - return false, "subzones", nil + // These need to be read in as a map as subzones can be added out-of-order. + type subzoneKey struct { + indexID uint32 + partitionName string } - for i, s := range z.Subzones { - o := other.Subzones[i] - if s.IndexID != o.IndexID { - return false, "subzone_index_id", nil + otherSubzonesBySubzoneKey := make(map[subzoneKey]Subzone, len(other.Subzones)) + for _, o := range other.Subzones { + k := subzoneKey{indexID: o.IndexID, partitionName: o.PartitionName} + otherSubzonesBySubzoneKey[k] = o + } + for _, s := range z.Subzones { + k := subzoneKey{indexID: s.IndexID, partitionName: s.PartitionName} + o, found := otherSubzonesBySubzoneKey[k] + if !found { + // There can be an extra zone config defined so long as + // it doesn't have any fields in the fieldList set. + if b, subzoneMismatch, err := s.Config.DiffWithZone( + *NewZoneConfig(), + fieldList, + ); err != nil { + return b, subzoneMismatch, err + } else if !b { + return false, DiffWithZoneMismatch{ + IndexID: s.IndexID, + PartitionName: s.PartitionName, + IsExtraSubzone: true, + }, nil + } + continue } - if s.PartitionName != o.PartitionName { - return false, "subzone_partition_name", nil + if b, subzoneMismatch, err := s.Config.DiffWithZone( + o.Config, + fieldList, + ); err != nil { + return b, subzoneMismatch, err + } else if !b { + // We should never have subzones nested within subzones. + if subzoneMismatch.IndexID > 0 { + return false, DiffWithZoneMismatch{}, errors.AssertionFailedf( + "unexpected subzone index id %d", + subzoneMismatch.IndexID, + ) + } + return b, DiffWithZoneMismatch{ + IndexID: o.IndexID, + PartitionName: o.PartitionName, + Field: subzoneMismatch.Field, + }, nil } - if b, str, err := s.Config.DiffWithZone(o.Config, fieldList); !b { - return b, str, err + delete(otherSubzonesBySubzoneKey, k) + } + + // Anything remaining in the map can be presumed to be missing. + // This is permitted provided that everything in the field list + // still matches on an empty zone configuration. + for _, o := range otherSubzonesBySubzoneKey { + if b, subzoneMismatch, err := NewZoneConfig().DiffWithZone( + o.Config, + fieldList, + ); err != nil { + return b, subzoneMismatch, err + } else if !b { + return false, DiffWithZoneMismatch{ + IndexID: o.IndexID, + PartitionName: o.PartitionName, + IsMissingSubzone: true, + }, nil } } - return true, "", nil + return true, DiffWithZoneMismatch{}, nil } // StoreSatisfiesConstraint checks whether a store satisfies the given constraint. diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 547c3c5ae9d4..b043e6742f1b 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -452,6 +452,10 @@ func dropZoneConfigsForMultiRegionIndexes( // should be marked as a placeholder config for a multi-region object. // See zonepb.IsSubzonePlaceholder for why this is necessary. func isPlaceholderZoneConfigForMultiRegion(zc zonepb.ZoneConfig) bool { + // Placeholders must have at least 1 subzone. + if len(zc.Subzones) == 0 { + return false + } // Strip Subzones / SubzoneSpans, as these may contain items if migrating // from one REGIONAL BY ROW table to another. strippedZC := zc @@ -567,7 +571,7 @@ func ApplyZoneConfigForMultiRegionTable( // in the zone config. This signifies a placeholder. // Note we do not use hasNewSubzones here as there may be existing subzones // on the zone config which may still be a placeholder. - if len(newZoneConfig.Subzones) > 0 && isPlaceholderZoneConfigForMultiRegion(newZoneConfig) { + if isPlaceholderZoneConfigForMultiRegion(newZoneConfig) { newZoneConfig.NumReplicas = proto.Int32(0) } @@ -863,7 +867,6 @@ func (p *planner) ValidateAllMultiRegionZoneConfigsInCurrentDatabase(ctx context ctx, dbDesc, tbDesc, - true, /* checkIndexZoneConfig */ &validateZoneConfigForMultiRegionErrorHandlerValidation{}, ) }, @@ -1120,23 +1123,30 @@ func (p *planner) CheckZoneConfigChangePermittedForMultiRegion( // an error to generate if validating a zone config for multi-region // fails. type validateZoneConfigForMultiRegionErrorHandler interface { - newError(descType string, descName string, field string) error + newMismatchFieldError(descType string, descName string, field string) error + newMissingSubzoneError(descType string, descName string) error + newExtraSubzoneError(descType string, descName string) error } // validateZoneConfigForMultiRegionErrorHandlerModifiedByUser implements // interface validateZoneConfigForMultiRegionErrorHandler. type validateZoneConfigForMultiRegionErrorHandlerModifiedByUser struct{} -func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) newError( +func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) newMismatchFieldError( descType string, descName string, field string, ) error { - err := pgerror.Newf( - pgcode.InvalidObjectDefinition, - "attempting to update zone configuration for %s %s which contains modified field %q", - descType, - descName, - field, + return v.wrapErr( + pgerror.Newf( + pgcode.InvalidObjectDefinition, + "attempting to update zone configuration for %s %s which contains modified field %q", + descType, + descName, + field, + ), ) +} + +func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) wrapErr(err error) error { err = errors.WithDetail( err, "the attempted operation will overwrite a user modified field", @@ -1148,11 +1158,37 @@ func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) newError( ) } +func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) newMissingSubzoneError( + descType string, descName string, +) error { + return v.wrapErr( + pgerror.Newf( + pgcode.InvalidObjectDefinition, + "attempting to update zone config which is missing an expected zone configuration for %s %s", + descType, + descName, + ), + ) +} + +func (v *validateZoneConfigForMultiRegionErrorHandlerModifiedByUser) newExtraSubzoneError( + descType string, descName string, +) error { + return v.wrapErr( + pgerror.Newf( + pgcode.InvalidObjectDefinition, + "attempting to update zone config which contains an extra zone configuration for %s %s", + descType, + descName, + ), + ) +} + // validateZoneConfigForMultiRegionErrorHandlerValidation implements // interface validateZoneConfigForMultiRegionErrorHandler. type validateZoneConfigForMultiRegionErrorHandlerValidation struct{} -func (v *validateZoneConfigForMultiRegionErrorHandlerValidation) newError( +func (v *validateZoneConfigForMultiRegionErrorHandlerValidation) newMismatchFieldError( descType string, descName string, field string, ) error { return pgerror.Newf( @@ -1164,6 +1200,28 @@ func (v *validateZoneConfigForMultiRegionErrorHandlerValidation) newError( ) } +func (v *validateZoneConfigForMultiRegionErrorHandlerValidation) newMissingSubzoneError( + descType string, descName string, +) error { + return pgerror.Newf( + pgcode.InvalidObjectDefinition, + "missing zone configuration for %s %s", + descType, + descName, + ) +} + +func (v *validateZoneConfigForMultiRegionErrorHandlerValidation) newExtraSubzoneError( + descType string, descName string, +) error { + return pgerror.Newf( + pgcode.InvalidObjectDefinition, + "extraneous zone configuration for %s %s", + descType, + descName, + ) +} + // validateZoneConfigForMultiRegionDatabaseWasNotModifiedByUser validates that // the zone configuration was not modified by the user. The function is intended // to be called in cases where a multi-region operation will overwrite the @@ -1204,16 +1262,19 @@ func (p *planner) validateZoneConfigForMultiRegionDatabase( return err } - same, field, err := currentZoneConfig.DiffWithZone(*expectedZoneConfig, zonepb.MultiRegionZoneConfigFields) + same, mismatch, err := currentZoneConfig.DiffWithZone( + *expectedZoneConfig, + zonepb.MultiRegionZoneConfigFields, + ) if err != nil { return err } if !same { dbName := tree.Name(dbDesc.GetName()) - return validateZoneConfigForMultiRegionErrorHandler.newError( + return validateZoneConfigForMultiRegionErrorHandler.newMismatchFieldError( "database", dbName.String(), - field, + mismatch.Field, ) } @@ -1242,7 +1303,6 @@ func (p *planner) validateZoneConfigForMultiRegionTableWasNotModifiedByUser( ctx, dbDesc, desc, - checkIndexZoneConfigs, &validateZoneConfigForMultiRegionErrorHandlerModifiedByUser{}, ) } @@ -1253,7 +1313,6 @@ func (p *planner) validateZoneConfigForMultiRegionTable( ctx context.Context, dbDesc *dbdesc.Immutable, desc catalog.TableDescriptor, - checkIndexZoneConfigs bool, validateZoneConfigForMultiRegionErrorHandler validateZoneConfigForMultiRegionErrorHandler, ) error { currentZoneConfig, err := getZoneConfigRaw(ctx, p.txn, p.ExecCfg().Codec, desc.GetID()) @@ -1271,43 +1330,8 @@ func (p *planner) validateZoneConfigForMultiRegionTable( tableName := tree.Name(desc.GetName()) - // TODO(#62790): we should check partition zone configs match on the - // validate zone config case. - if checkIndexZoneConfigs { - // Check to see if the to be applied zone configurations will override - // any existing zone configurations on the table's indexes. - // We say "override" here, and not "overwrite" because - // REGIONAL BY ROW tables will not write zone configs at the index level, - // but instead, at the index partition level. That being said, application - // of a partition-level zone config will override any applied index-level - // zone config, so it's important that we warn the user of that. - for _, s := range currentZoneConfig.Subzones { - if s.PartitionName == "" { - // Found a zone config on an index. Check to see if any of its - // multi-region fields are set. - if isSet, field := s.Config.IsAnyMultiRegionFieldSet(); isSet { - // Find the name of the offending index to use in the message below. - // In the case where we can't find the name, do our best and return - // the ID. - indexName := fmt.Sprintf("[%d]", s.IndexID) - for _, i := range desc.ActiveIndexes() { - if uint32(i.GetID()) == s.IndexID { - indexTreeName := tree.Name(i.GetName()) - indexName = indexTreeName.String() - } - } - return validateZoneConfigForMultiRegionErrorHandler.newError( - "index", - fmt.Sprintf("%s@%s", tableName.String(), indexName), - field, - ) - } - } - } - } - _, expectedZoneConfig, err := ApplyZoneConfigForMultiRegionTableOptionTableAndIndexes( - *currentZoneConfig, + *zonepb.NewZoneConfig(), regionConfig, desc, ) @@ -1315,14 +1339,42 @@ func (p *planner) validateZoneConfigForMultiRegionTable( return err } - // Mark the NumReplicas as 0 if we have subzones but no other features - // in the zone config. This signifies a placeholder. - if len(expectedZoneConfig.Subzones) > 0 && isPlaceholderZoneConfigForMultiRegion(expectedZoneConfig) { + // Some inactive subzones may remain on the zone configuration until it is cleaned up + // at a later step. Keep track of all active indexes from the descriptor. + activeSubzoneIndexIDs := make(map[uint32]tree.Name, len(desc.ActiveIndexes())) + for _, idx := range desc.ActiveIndexes() { + activeSubzoneIndexIDs[uint32(idx.GetID())] = tree.Name(idx.GetName()) + } + + // Remove inactive subzones from the comparison. + filteredSubzones := currentZoneConfig.Subzones[:0] + for _, c := range currentZoneConfig.Subzones { + if _, ok := activeSubzoneIndexIDs[c.IndexID]; ok { + filteredSubzones = append(filteredSubzones, c) + } + } + currentZoneConfig.Subzones = filteredSubzones + // Strip the placeholder status if there are no active subzones on the current + // zone config. + if len(filteredSubzones) == 0 && currentZoneConfig.IsSubzonePlaceholder() { + currentZoneConfig.NumReplicas = nil + } + // 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 + // are set on the current zone config, the expected zone config needs + // the placeholder marked so that DiffWithZone does not error when + // num_replicas is expectedly different. + // e.g. if current zone config has gc.ttlseconds set, then we + // do not fudge num replicas to be equal to 0 -- otherwise the + // check fails when num_replicas is different, but that is + // expected as the current zone config is no longer a placeholder. + if currentZoneConfig.IsSubzonePlaceholder() && isPlaceholderZoneConfigForMultiRegion(expectedZoneConfig) { expectedZoneConfig.NumReplicas = proto.Int32(0) } // Compare the two zone configs to see if anything is amiss. - same, field, err := currentZoneConfig.DiffWithZone( + same, mismatch, err := currentZoneConfig.DiffWithZone( expectedZoneConfig, zonepb.MultiRegionZoneConfigFields, ) @@ -1330,10 +1382,49 @@ func (p *planner) validateZoneConfigForMultiRegionTable( return err } if !same { - return validateZoneConfigForMultiRegionErrorHandler.newError( - "table", - tableName.String(), - field, + descType := "table" + name := tableName.String() + if mismatch.IndexID != 0 { + indexName, ok := activeSubzoneIndexIDs[mismatch.IndexID] + if !ok { + return errors.AssertionFailedf( + "unexpected unknown index id %d on table %s", + mismatch.IndexID, + tableName, + ) + } + + if mismatch.PartitionName != "" { + descType = "partition" + partitionName := tree.Name(mismatch.PartitionName) + name = fmt.Sprintf( + "%s of %s@%s", + partitionName.String(), + tableName.String(), + indexName.String(), + ) + } else { + descType = "index" + name = fmt.Sprintf("%s@%s", tableName.String(), indexName.String()) + } + } + + if mismatch.IsMissingSubzone { + return validateZoneConfigForMultiRegionErrorHandler.newMissingSubzoneError( + descType, + name, + ) + } + if mismatch.IsExtraSubzone { + return validateZoneConfigForMultiRegionErrorHandler.newExtraSubzoneError( + descType, + name, + ) + } + return validateZoneConfigForMultiRegionErrorHandler.newMismatchFieldError( + descType, + name, + mismatch.Field, ) }