From 3e1b61d0bd449f130d1e34f332b53b3a07460ad7 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Tue, 30 Mar 2021 22:34:16 +1100 Subject: [PATCH] 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 is correctly configured. --- docs/generated/sql/functions.md | 13 +- 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 | 12 + pkg/sql/region_util.go | 207 +++++++++++++----- pkg/sql/sem/builtins/builtins.go | 21 +- pkg/sql/sem/tree/eval.go | 5 + 8 files changed, 236 insertions(+), 65 deletions(-) diff --git a/docs/generated/sql/functions.md b/docs/generated/sql/functions.md index 30a6995816cc..f21f7e5f4823 100644 --- a/docs/generated/sql/functions.md +++ b/docs/generated/sql/functions.md @@ -873,6 +873,14 @@ 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 set +up for the current database. Returns an error if validation fails.

+
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 +2380,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..3282963ba48b 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) } +// ValidateCurrentDatabaseMultiRegionZoneConfigs is part of the tree.EvalDatabase interface. +func (so *importSequenceOperators) ValidateCurrentDatabaseMultiRegionZoneConfigs( + _ 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 78706489b37f..5218fb14838f 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..c518b9f2c53d 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) } +// ValidateCurrentDatabaseMultiRegionZoneConfigs is part of the tree.EvalDatabase interface. +func (so *DummySequenceOperators) ValidateCurrentDatabaseMultiRegionZoneConfigs( + _ 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,11 @@ func (ep *DummyEvalPlanner) CurrentDatabaseRegionConfig( return nil, errors.WithStack(errEvalPlanner) } +// ValidateCurrentDatabaseMultiRegionZoneConfigs is part of the tree.EvalDatabase interface. +func (ep *DummyEvalPlanner) ValidateCurrentDatabaseMultiRegionZoneConfigs(_ 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..4d5f7dcf7270 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( } } +// ValidateCurrentDatabaseMultiRegionZoneConfigs is part of the tree.EvalDatabase interface. +// and as such is intended for DML use. It returns an empty DatabaseRegionConfig +// if the current database is not multi-region enabled. +func (p *planner) ValidateCurrentDatabaseMultiRegionZoneConfigs(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 @@ -1076,6 +1114,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 +1174,21 @@ 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 the exact expectations 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 +1208,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 +1229,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 +1237,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 +1263,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 +1284,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 e99a79117d7c..fd92a5a86fcf 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -5063,8 +5063,27 @@ 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.ValidateCurrentDatabaseMultiRegionZoneConfigs( + evalCtx.Context, + ); err != nil { + return nil, err + } + return tree.MakeDBool(true), nil + }, + Info: `Validates all multi-region zone configurations are correctly set + up for the current database. Returns an error if validation fails. + `, + Volatility: tree.VolatilityStable, + }, + ), "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 b624043f6db0..c5bb3faf9ca1 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) + // ValidateCurrentDatabaseMultiRegionZoneConfigs validates whether the current + // database's multi-region zone configs are correctly setup. This includes + // all tables within the database. + ValidateCurrentDatabaseMultiRegionZoneConfigs(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.