From 5f5101b0ac9e15aa9bf86259353cf350adfd8c6f Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 1 Apr 2021 06:50:18 +1100 Subject: [PATCH] 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, ) }