From da1be84795675d2a777618e28468c70623bd524e Mon Sep 17 00:00:00 2001 From: Solon Gordon Date: Thu, 8 Aug 2019 13:03:47 -0400 Subject: [PATCH] sql: support wildcard in ALTER PARTITION OF INDEX `ALTER PARTITION ... OF INDEX` now allows a user to specify all indexes of a table via `tbl@*` syntax. This means that every partition with the specified name across all of a table's indexes should be affected. Refers #39357 Release note (sql change): The `ALTER PARTITION` statement now supports applying a zone configuration to all the partitions of a table and its indexes that share the same partition name. The syntax for this is `ALTER PARTITION OF INDEX @*`. --- .../sql/bnf/alter_zone_index_stmt.bnf | 6 - .../sql/bnf/alter_zone_partition_stmt.bnf | 13 + .../sql/bnf/alter_zone_table_stmt.bnf | 3 - docs/generated/sql/bnf/stmt_block.bnf | 11 +- pkg/ccl/logictestccl/testdata/logic_test/zone | 27 + pkg/cmd/docgen/diagrams.go | 6 + pkg/sql/parser/sql.y | 71 ++- pkg/sql/sem/tree/zone.go | 8 + pkg/sql/set_zone_config.go | 510 ++++++++++-------- 9 files changed, 387 insertions(+), 268 deletions(-) create mode 100644 docs/generated/sql/bnf/alter_zone_partition_stmt.bnf diff --git a/docs/generated/sql/bnf/alter_zone_index_stmt.bnf b/docs/generated/sql/bnf/alter_zone_index_stmt.bnf index 268cfcffb270..b05ef5887525 100644 --- a/docs/generated/sql/bnf/alter_zone_index_stmt.bnf +++ b/docs/generated/sql/bnf/alter_zone_index_stmt.bnf @@ -5,9 +5,3 @@ alter_zone_index_stmt ::= | 'ALTER' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* | 'ALTER' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* | 'ALTER' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'DISCARD' - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'DISCARD' - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'DISCARD' diff --git a/docs/generated/sql/bnf/alter_zone_partition_stmt.bnf b/docs/generated/sql/bnf/alter_zone_partition_stmt.bnf new file mode 100644 index 000000000000..a5c8e40ba2e5 --- /dev/null +++ b/docs/generated/sql/bnf/alter_zone_partition_stmt.bnf @@ -0,0 +1,13 @@ +alter_zone_partition_stmt ::= + 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'DISCARD' + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' index_name 'CONFIGURE' 'ZONE' 'DISCARD' + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' index_name 'CONFIGURE' 'ZONE' 'DISCARD' + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' '*' 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' '*' 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' '*' 'CONFIGURE' 'ZONE' 'DISCARD' diff --git a/docs/generated/sql/bnf/alter_zone_table_stmt.bnf b/docs/generated/sql/bnf/alter_zone_table_stmt.bnf index 6f541c683d4c..b5e8b8c5b798 100644 --- a/docs/generated/sql/bnf/alter_zone_table_stmt.bnf +++ b/docs/generated/sql/bnf/alter_zone_table_stmt.bnf @@ -2,6 +2,3 @@ alter_zone_table_stmt ::= 'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* | 'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* | 'ALTER' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'DISCARD' - | 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' 'COPY' 'FROM' 'PARENT' ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* - | 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'USING' variable '=' value ( ( ',' variable '=' value | ',' variable '=' 'COPY' 'FROM' 'PARENT' ) )* - | 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name 'CONFIGURE' 'ZONE' 'DISCARD' diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 38ec03a4d040..e6e826d2349a 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -275,6 +275,7 @@ abort_stmt ::= alter_ddl_stmt ::= alter_table_stmt + | alter_partition_stmt | alter_index_stmt | alter_view_stmt | alter_sequence_stmt @@ -939,6 +940,9 @@ alter_table_stmt ::= | alter_zone_table_stmt | alter_rename_table_stmt +alter_partition_stmt ::= + alter_zone_partition_stmt + alter_index_stmt ::= alter_oneindex_stmt | alter_split_index_stmt @@ -1252,12 +1256,16 @@ alter_scatter_stmt ::= alter_zone_table_stmt ::= 'ALTER' 'TABLE' table_name set_zone_config - | 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name set_zone_config alter_rename_table_stmt ::= 'ALTER' 'TABLE' relation_expr 'RENAME' 'TO' table_name | 'ALTER' 'TABLE' 'IF' 'EXISTS' relation_expr 'RENAME' 'TO' table_name +alter_zone_partition_stmt ::= + 'ALTER' 'PARTITION' partition_name 'OF' 'TABLE' table_name set_zone_config + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_index_name set_zone_config + | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_name '@' '*' set_zone_config + alter_oneindex_stmt ::= 'ALTER' 'INDEX' table_index_name alter_index_cmds | 'ALTER' 'INDEX' 'IF' 'EXISTS' table_index_name alter_index_cmds @@ -1280,7 +1288,6 @@ alter_rename_index_stmt ::= alter_zone_index_stmt ::= 'ALTER' 'INDEX' table_index_name set_zone_config - | 'ALTER' 'PARTITION' partition_name 'OF' 'INDEX' table_index_name set_zone_config alter_rename_view_stmt ::= 'ALTER' 'VIEW' relation_expr 'RENAME' 'TO' view_name diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/pkg/ccl/logictestccl/testdata/logic_test/zone index 604ae51f8f57..853026b3e277 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/zone +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone @@ -566,3 +566,30 @@ ALTER PARTITION p1 OF INDEX "my database".public.show_test@primary CONFIGURE ZON constraints = '[+dc=dc1]'; ALTER PARTITION p2 OF INDEX "my database".public.show_test@primary CONFIGURE ZONE USING constraints = '[+dc=dc2]' + +subtest alter_partition_across_all_indexes + +statement ok +CREATE TABLE t2 (x INT PRIMARY KEY) PARTITION BY LIST (x) ( + PARTITION p1 VALUES IN (1), + PARTITION p2 VALUES IN (2) +); +CREATE INDEX x1 ON t2 (x) PARTITION BY LIST (x) ( + PARTITION p1 VALUES IN (1), + PARTITION p2 VALUES IN (2) +); +CREATE INDEX x2 ON t2 (x) PARTITION BY LIST (x) ( + PARTITION p1 VALUES IN (1), + PARTITION p2 VALUES IN (2) +); +ALTER PARTITION p1 OF INDEX t2@* CONFIGURE ZONE USING num_replicas = 1 + +query TT +SELECT * FROM [SHOW ALL ZONE CONFIGURATIONS] WHERE target LIKE '%t2@%' +---- +PARTITION p1 OF INDEX "my database".public.t2@primary ALTER PARTITION p1 OF INDEX "my database".public.t2@primary CONFIGURE ZONE USING + num_replicas = 1 +PARTITION p1 OF INDEX "my database".public.t2@x1 ALTER PARTITION p1 OF INDEX "my database".public.t2@x1 CONFIGURE ZONE USING + num_replicas = 1 +PARTITION p1 OF INDEX "my database".public.t2@x2 ALTER PARTITION p1 OF INDEX "my database".public.t2@x2 CONFIGURE ZONE USING + num_replicas = 1 diff --git a/pkg/cmd/docgen/diagrams.go b/pkg/cmd/docgen/diagrams.go index fb3b8ff18473..e18d9620380a 100644 --- a/pkg/cmd/docgen/diagrams.go +++ b/pkg/cmd/docgen/diagrams.go @@ -417,6 +417,12 @@ var specs = []stmtSpec{ replace: map[string]string{"var_name": "variable", "var_value": "value"}, unlink: []string{"variable", "value"}, }, + { + name: "alter_zone_partition_stmt", + inline: []string{"table_index_name", "set_zone_config", "var_set_list"}, + replace: map[string]string{"var_name": "variable", "var_value": "value", "standalone_index_name": "index_name"}, + unlink: []string{"variable", "value"}, + }, { name: "backup", stmt: "backup_stmt", diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index 2ae27c81a6f2..cdd8021dc446 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -604,6 +604,7 @@ func newNameFromStr(s string) *tree.Name { %type alter_database_stmt %type alter_user_stmt %type alter_range_stmt +%type alter_partition_stmt // ALTER RANGE %type alter_zone_range_stmt @@ -618,6 +619,9 @@ func newNameFromStr(s string) *tree.Name { %type alter_relocate_lease_stmt %type alter_zone_table_stmt +// ALTER PARTITION +%type alter_zone_partition_stmt + // ALTER DATABASE %type alter_rename_database_stmt %type alter_zone_database_stmt @@ -1096,12 +1100,13 @@ alter_stmt: | ALTER error // SHOW HELP: ALTER alter_ddl_stmt: - alter_table_stmt // EXTEND WITH HELP: ALTER TABLE -| alter_index_stmt // EXTEND WITH HELP: ALTER INDEX -| alter_view_stmt // EXTEND WITH HELP: ALTER VIEW -| alter_sequence_stmt // EXTEND WITH HELP: ALTER SEQUENCE -| alter_database_stmt // EXTEND WITH HELP: ALTER DATABASE -| alter_range_stmt // EXTEND WITH HELP: ALTER RANGE + alter_table_stmt // EXTEND WITH HELP: ALTER TABLE +| alter_index_stmt // EXTEND WITH HELP: ALTER INDEX +| alter_view_stmt // EXTEND WITH HELP: ALTER VIEW +| alter_sequence_stmt // EXTEND WITH HELP: ALTER SEQUENCE +| alter_database_stmt // EXTEND WITH HELP: ALTER DATABASE +| alter_range_stmt // EXTEND WITH HELP: ALTER RANGE +| alter_partition_stmt // EXTEND WITH HELP: ALTER PARTITION // %Help: ALTER TABLE - change the definition of a table // %Category: DDL @@ -1129,7 +1134,6 @@ alter_ddl_stmt: // ALTER TABLE ... PARTITION BY LIST ( ) ( ) // ALTER TABLE ... PARTITION BY NOTHING // ALTER TABLE ... CONFIGURE ZONE -// ALTER PARTITION ... OF TABLE ... CONFIGURE ZONE // // Column qualifiers: // [CONSTRAINT ] {NULL | NOT NULL | UNIQUE | PRIMARY KEY | CHECK () | DEFAULT } @@ -1156,7 +1160,26 @@ alter_table_stmt: // ALTER TABLE has its error help token here because the ALTER TABLE // prefix is spread over multiple non-terminals. | ALTER TABLE error // SHOW HELP: ALTER TABLE -| ALTER PARTITION error // SHOW HELP: ALTER TABLE + +// %Help: ALTER PARTITION - apply zone configurations to a partition +// %Category: DDL +// %Text: +// ALTER PARTITION +// +// Commands: +// ALTER PARTITION ... OF TABLE ... CONFIGURE ZONE +// ALTER PARTITION ... OF INDEX ... CONFIGURE ZONE +// +// Zone configurations: +// DISCARD +// USING = [, ...] +// USING = COPY FROM PARENT [, ...] +// { TO | = } +// +// %SeeAlso: WEBDOCS/configure-zone.html +alter_partition_stmt: + alter_zone_partition_stmt +| ALTER PARTITION error // SHOW HELP: ALTER PARTITION // %Help: ALTER VIEW - change the definition of a view // %Category: DDL @@ -1245,7 +1268,6 @@ alter_range_stmt: // ALTER INDEX ... UNSPLIT AT // ALTER INDEX ... UNSPLIT ALL // ALTER INDEX ... SCATTER [ FROM ( ) TO ( ) ] -// ALTER PARTITION ... OF INDEX ... CONFIGURE ZONE // // Zone configurations: // DISCARD @@ -1432,23 +1454,25 @@ alter_zone_table_stmt: } $$.val = s } -| ALTER PARTITION partition_name OF TABLE table_name set_zone_config + +alter_zone_index_stmt: + ALTER INDEX table_index_name set_zone_config { - name := $6.unresolvedObjectName().ToTableName() - s := $7.setZoneConfig() + s := $4.setZoneConfig() s.ZoneSpecifier = tree.ZoneSpecifier{ - TableOrIndex: tree.TableIndexName{Table: name}, - Partition: tree.Name($3), + TableOrIndex: $3.tableIndexName(), } $$.val = s } -alter_zone_index_stmt: - ALTER INDEX table_index_name set_zone_config +alter_zone_partition_stmt: + ALTER PARTITION partition_name OF TABLE table_name set_zone_config { - s := $4.setZoneConfig() + name := $6.unresolvedObjectName().ToTableName() + s := $7.setZoneConfig() s.ZoneSpecifier = tree.ZoneSpecifier{ - TableOrIndex: $3.tableIndexName(), + TableOrIndex: tree.TableIndexName{Table: name}, + Partition: tree.Name($3), } $$.val = s } @@ -1461,6 +1485,17 @@ alter_zone_index_stmt: } $$.val = s } +| ALTER PARTITION partition_name OF INDEX table_name '@' '*' set_zone_config + { + name := $6.unresolvedObjectName().ToTableName() + s := $9.setZoneConfig() + s.ZoneSpecifier = tree.ZoneSpecifier{ + TableOrIndex: tree.TableIndexName{Table: name}, + Partition: tree.Name($3), + } + s.AllIndexes = true + $$.val = s + } var_set_list: var_name '=' COPY FROM PARENT diff --git a/pkg/sql/sem/tree/zone.go b/pkg/sql/sem/tree/zone.go index 9e5be193405a..1776129caa28 100644 --- a/pkg/sql/sem/tree/zone.go +++ b/pkg/sql/sem/tree/zone.go @@ -34,6 +34,11 @@ func (node ZoneSpecifier) TargetsIndex() bool { return node.TargetsTable() && node.TableOrIndex.Index != "" } +// TargetsPartition returns whether the zone specifier targets a partition. +func (node ZoneSpecifier) TargetsPartition() bool { + return node.TargetsTable() && node.Partition != "" +} + // Format implements the NodeFormatter interface. func (node *ZoneSpecifier) Format(ctx *FmtCtx) { if node.NamedZone != "" { @@ -79,6 +84,9 @@ func (node *ShowZoneConfig) Format(ctx *FmtCtx) { // statement. type SetZoneConfig struct { ZoneSpecifier + // AllIndexes indicates that the zone configuration should be applied across + // all of a tables indexes. (ALTER PARTITION ... OF INDEX @*) + AllIndexes bool SetDefault bool YAMLConfig Expr Options KVOptions diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index ae552cf1b6e1..d921473d215c 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -28,7 +28,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" "github.com/gogo/protobuf/proto" - yaml "gopkg.in/yaml.v2" + "gopkg.in/yaml.v2" ) type optionValue struct { @@ -38,6 +38,7 @@ type optionValue struct { type setZoneConfigNode struct { zoneSpecifier tree.ZoneSpecifier + allIndexes bool yamlConfig tree.TypedExpr options map[tree.Name]optionValue setDefault bool @@ -150,6 +151,7 @@ func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (pla return &setZoneConfigNode{ zoneSpecifier: n.ZoneSpecifier, + allIndexes: n.AllIndexes, yamlConfig: yamlConfig, options: options, setDefault: n.SetDefault, @@ -241,276 +243,306 @@ func (n *setZoneConfigNode) startExec(params runParams) error { return err } - // resolveZone determines the ID of the target object of the zone - // specifier. This ought to succeed regardless of whether there is - // already a zone config for the target object. - targetID, err := resolveZone( - params.ctx, params.p.txn, &n.zoneSpecifier) - if err != nil { - return err - } - if targetID != keys.SystemDatabaseID && sqlbase.IsSystemConfigID(targetID) { - return pgerror.Newf(pgcode.CheckViolation, - `cannot set zone configs for system config tables; `+ - `try setting your config on the entire "system" database instead`) - } else if targetID == keys.RootNamespaceID && deleteZone { - return pgerror.Newf(pgcode.CheckViolation, - "cannot remove default zone") - } - - // resolveSubzone determines the sub-parts of the zone - // specifier. This ought to succeed regardless of whether there is - // already a zone config. - index, partition, err := resolveSubzone(&n.zoneSpecifier, table) - if err != nil { - return err - } - - // Retrieve the partial zone configuration - partialZone, err := getZoneConfigRaw(params.ctx, params.p.txn, targetID) - if err != nil { - return err + // If this is an ALTER ALL PARTITIONS statement, we need to find all indexes + // with the specified partition name and apply the zone configuration to all + // of them. + var specifiers []tree.ZoneSpecifier + if n.zoneSpecifier.TargetsPartition() && n.allIndexes { + for _, idx := range table.AllNonDropIndexes() { + if p := idx.FindPartitionByName(string(n.zoneSpecifier.Partition)); p != nil { + zs := n.zoneSpecifier + zs.TableOrIndex.Index = tree.UnrestrictedName(idx.Name) + specifiers = append(specifiers, zs) + } + } + } else { + specifiers = append(specifiers, n.zoneSpecifier) } - // No zone was found. Possibly a SubzonePlaceholder depending on the index. - if partialZone == nil { - partialZone = config.NewZoneConfig() - if index != nil { - subzonePlaceholder = true + applyZoneConfig := func(zs tree.ZoneSpecifier) error { + // resolveZone determines the ID of the target object of the zone + // specifier. This ought to succeed regardless of whether there is + // already a zone config for the target object. + targetID, err := resolveZone(params.ctx, params.p.txn, &zs) + if err != nil { + return err + } + if targetID != keys.SystemDatabaseID && sqlbase.IsSystemConfigID(targetID) { + return pgerror.Newf(pgcode.CheckViolation, + `cannot set zone configs for system config tables; `+ + `try setting your config on the entire "system" database instead`) + } else if targetID == keys.RootNamespaceID && deleteZone { + return pgerror.Newf(pgcode.CheckViolation, + "cannot remove default zone") } - } - var partialSubzone *config.Subzone - if index != nil { - partialSubzone = partialZone.GetSubzone(uint32(index.ID), partition) - if partialSubzone == nil { - partialSubzone = &config.Subzone{Config: *config.NewZoneConfig()} + // resolveSubzone determines the sub-parts of the zone + // specifier. This ought to succeed regardless of whether there is + // already a zone config. + index, partition, err := resolveSubzone(&zs, table) + if err != nil { + return err } - } - // Retrieve the zone configuration. - // - // If the statement was USING DEFAULT, we want to ignore the zone - // config that exists on targetID and instead skip to the inherited - // default (whichever applies -- a database if targetID is a table, - // default if targetID is a database, etc.). For this, we use the last - // parameter getInheritedDefault to GetZoneConfigInTxn(). - // These zones are only used for validations. The merged zone is will - // not be written. - _, completeZone, completeSubzone, err := GetZoneConfigInTxn(params.ctx, params.p.txn, - uint32(targetID), index, partition, n.setDefault) - - if err == errNoZoneConfigApplies { - // No zone config yet. - // - // GetZoneConfigInTxn will fail with errNoZoneConfigApplies when - // the target ID is not a database object, i.e. one of the system - // ranges (liveness, meta, etc.), and did not have a zone config - // already. - completeZone = protoutil.Clone(params.extendedEvalCtx.ExecCfg.DefaultZoneConfig).(*config.ZoneConfig) - } else if err != nil { - return err - } + // Retrieve the partial zone configuration + partialZone, err := getZoneConfigRaw(params.ctx, params.p.txn, targetID) + if err != nil { + return err + } - // Copy the fields set by the INHERIT field command. - partialZone.CopyFromZone(*completeZone, copyFromParentList) + // No zone was found. Possibly a SubzonePlaceholder depending on the index. + if partialZone == nil { + partialZone = config.NewZoneConfig() + if index != nil { + subzonePlaceholder = true + } + } - if deleteZone { + var partialSubzone *config.Subzone if index != nil { - didDelete := completeZone.DeleteSubzone(uint32(index.ID), partition) - _ = partialZone.DeleteSubzone(uint32(index.ID), partition) - if !didDelete { - // If we didn't do any work, return early. We'd otherwise perform an - // update that would make it look like one row was affected. - return nil + partialSubzone = partialZone.GetSubzone(uint32(index.ID), partition) + if partialSubzone == nil { + partialSubzone = &config.Subzone{Config: *config.NewZoneConfig()} } - } else { - completeZone.DeleteTableConfig() - partialZone.DeleteTableConfig() - } - } else { - // Validate the user input. - if len(yamlConfig) == 0 || yamlConfig[len(yamlConfig)-1] != '\n' { - // YAML values must always end with a newline character. If there is none, - // for UX convenience add one. - yamlConfig += "\n" } - // Determine where to load the configuration. - newZone := *completeZone - if completeSubzone != nil { - newZone = completeSubzone.Config + // Retrieve the zone configuration. + // + // If the statement was USING DEFAULT, we want to ignore the zone + // config that exists on targetID and instead skip to the inherited + // default (whichever applies -- a database if targetID is a table, + // default if targetID is a database, etc.). For this, we use the last + // parameter getInheritedDefault to GetZoneConfigInTxn(). + // These zones are only used for validations. The merged zone is will + // not be written. + _, completeZone, completeSubzone, err := GetZoneConfigInTxn(params.ctx, params.p.txn, + uint32(targetID), index, partition, n.setDefault) + + if err == errNoZoneConfigApplies { + // No zone config yet. + // + // GetZoneConfigInTxn will fail with errNoZoneConfigApplies when + // the target ID is not a database object, i.e. one of the system + // ranges (liveness, meta, etc.), and did not have a zone config + // already. + completeZone = protoutil.Clone(params.extendedEvalCtx.ExecCfg.DefaultZoneConfig).(*config.ZoneConfig) + } else if err != nil { + return err } - // Determine where to load the partial configuration. - // finalZone is where the new changes are unmarshalled onto. - // It must be a fresh ZoneConfig if a new subzone is being created. - // If an existing subzone is being modified, finalZone is overridden. - finalZone := *partialZone - if partialSubzone != nil { - finalZone = partialSubzone.Config - } + // Copy the fields set by the INHERIT field command. + partialZone.CopyFromZone(*completeZone, copyFromParentList) + + if deleteZone { + if index != nil { + didDelete := completeZone.DeleteSubzone(uint32(index.ID), partition) + _ = partialZone.DeleteSubzone(uint32(index.ID), partition) + if !didDelete { + // If we didn't do any work, return early. We'd otherwise perform an + // update that would make it look like one row was affected. + return nil + } + } else { + completeZone.DeleteTableConfig() + partialZone.DeleteTableConfig() + } + } else { + // Validate the user input. + if len(yamlConfig) == 0 || yamlConfig[len(yamlConfig)-1] != '\n' { + // YAML values must always end with a newline character. If there is none, + // for UX convenience add one. + yamlConfig += "\n" + } - // ALTER RANGE default USING DEFAULT sets the default to the in - // memory default value. - if n.setDefault && keys.RootNamespaceID == uint32(targetID) { - finalZone = *protoutil.Clone(params.extendedEvalCtx.ExecCfg.DefaultZoneConfig).(*config.ZoneConfig) - } else if n.setDefault { - finalZone = *config.NewZoneConfig() - } - // Load settings from YAML. If there was no YAML (e.g. because the - // query specified CONFIGURE ZONE USING), the YAML string will be - // empty, in which case the unmarshaling will be a no-op. This is - // innocuous. - if err := yaml.UnmarshalStrict([]byte(yamlConfig), &newZone); err != nil { - return pgerror.Newf(pgcode.CheckViolation, - "could not parse zone config: %v", err) - } + // Determine where to load the configuration. + newZone := *completeZone + if completeSubzone != nil { + newZone = completeSubzone.Config + } - // Load settings from YAML into the partial zone as well. - if err := yaml.UnmarshalStrict([]byte(yamlConfig), &finalZone); err != nil { - return pgerror.Newf(pgcode.CheckViolation, - "could not parse zone config: %v", err) - } + // Determine where to load the partial configuration. + // finalZone is where the new changes are unmarshalled onto. + // It must be a fresh ZoneConfig if a new subzone is being created. + // If an existing subzone is being modified, finalZone is overridden. + finalZone := *partialZone + if partialSubzone != nil { + finalZone = partialSubzone.Config + } + + // ALTER RANGE default USING DEFAULT sets the default to the in + // memory default value. + if n.setDefault && keys.RootNamespaceID == uint32(targetID) { + finalZone = *protoutil.Clone(params.extendedEvalCtx.ExecCfg.DefaultZoneConfig).(*config.ZoneConfig) + } else if n.setDefault { + finalZone = *config.NewZoneConfig() + } + // Load settings from YAML. If there was no YAML (e.g. because the + // query specified CONFIGURE ZONE USING), the YAML string will be + // empty, in which case the unmarshaling will be a no-op. This is + // innocuous. + if err := yaml.UnmarshalStrict([]byte(yamlConfig), &newZone); err != nil { + return pgerror.Newf(pgcode.CheckViolation, + "could not parse zone config: %v", err) + } + + // Load settings from YAML into the partial zone as well. + if err := yaml.UnmarshalStrict([]byte(yamlConfig), &finalZone); err != nil { + return pgerror.Newf(pgcode.CheckViolation, + "could not parse zone config: %v", err) + } - // Load settings from var = val assignments. If there were no such - // settings, (e.g. because the query specified CONFIGURE ZONE = or - // USING DEFAULT), the setter slice will be empty and this will be - // a no-op. This is innocuous. - for _, setter := range setters { - // A setter may fail with an error-via-panic. Catch those. - if err := func() (err error) { - defer func() { - if p := recover(); p != nil { - if errP, ok := p.(error); ok { - // Catch and return the error. - err = errP - } else { - // Nothing we know about, let it continue as a panic. - panic(p) + // Load settings from var = val assignments. If there were no such + // settings, (e.g. because the query specified CONFIGURE ZONE = or + // USING DEFAULT), the setter slice will be empty and this will be + // a no-op. This is innocuous. + for _, setter := range setters { + // A setter may fail with an error-via-panic. Catch those. + if err := func() (err error) { + defer func() { + if p := recover(); p != nil { + if errP, ok := p.(error); ok { + // Catch and return the error. + err = errP + } else { + // Nothing we know about, let it continue as a panic. + panic(p) + } } - } - }() + }() + + setter(&newZone) + setter(&finalZone) + return nil + }(); err != nil { + return err + } + } + + // Validate that there are no conflicts in the zone setup. + if err := validateNoRepeatKeysInZone(&newZone); err != nil { + return err + } - setter(&newZone) - setter(&finalZone) - return nil - }(); err != nil { + // Validate that the result makes sense. + if err := validateZoneAttrsAndLocalities( + params.ctx, + params.extendedEvalCtx.StatusServer.Nodes, + &newZone, + ); err != nil { return err } + + // Are we operating on an index? + if index == nil { + // No: the final zone config is the one we just processed. + completeZone = &newZone + partialZone = &finalZone + } else { + // If the zone config for targetID was a subzone placeholder, it'll have + // been skipped over by GetZoneConfigInTxn. We need to load it regardless + // to avoid blowing away other subzones. + + // TODO(ridwanmsharif): How is this supposed to change? getZoneConfigRaw + // gives no guarantees about completeness. Some work might need to happen + // here to complete the missing fields. The reason is because we don't know + // here if a zone is a placeholder or not. Can we do a GetConfigInTxn here? + // And if it is a placeholder, we use getZoneConfigRaw to create one. + completeZone, err = getZoneConfigRaw(params.ctx, params.p.txn, targetID) + if err != nil { + return err + } else if completeZone == nil { + completeZone = config.NewZoneConfig() + } + completeZone.SetSubzone(config.Subzone{ + IndexID: uint32(index.ID), + PartitionName: partition, + Config: newZone, + }) + + // The partial zone might just be empty. If so, + // replace it with a SubzonePlaceholder. + if subzonePlaceholder { + partialZone.DeleteTableConfig() + } + + partialZone.SetSubzone(config.Subzone{ + IndexID: uint32(index.ID), + PartitionName: partition, + Config: finalZone, + }) + } + + // Finally revalidate everything. Validate only the completeZone config. + if err := completeZone.Validate(); err != nil { + return pgerror.Newf(pgcode.CheckViolation, + "could not validate zone config: %v", err) + } } - // Validate that there are no conflicts in the zone setup. - if err := validateNoRepeatKeysInZone(&newZone); err != nil { + // Write the partial zone configuration. + hasNewSubzones := !deleteZone && index != nil + execConfig := params.extendedEvalCtx.ExecCfg + zoneToWrite := partialZone + + // Finally check for the extra protection partial zone configs would + // require from changes made to parent zones. The extra protections are: + // + // RangeMinBytes and RangeMaxBytes must be set together + // LeasePreferences cannot be set unless Constraints are explicitly set + // Per-replica constraints cannot be set unless num_replicas is explicitly set + if err := zoneToWrite.ValidateTandemFields(); err != nil { + err = errors.Wrap(err, "could not validate zone config") + err = pgerror.WithCandidateCode(err, pgcode.InvalidParameterValue) + err = errors.WithHint(err, + "try ALTER ... CONFIGURE ZONE USING = COPY FROM PARENT [, ...] to populate the field") return err } - - // Validate that the result makes sense. - if err := validateZoneAttrsAndLocalities( - params.ctx, - params.extendedEvalCtx.StatusServer.Nodes, - &newZone, - ); err != nil { + n.run.numAffected, err = writeZoneConfig(params.ctx, params.p.txn, + targetID, table, zoneToWrite, execConfig, hasNewSubzones) + if err != nil { return err } - // Are we operating on an index? - if index == nil { - // No: the final zone config is the one we just processed. - completeZone = &newZone - partialZone = &finalZone + // Record that the change has occurred for auditing. + var eventLogType EventLogType + info := struct { + Target string + Config string `json:",omitempty"` + Options string `json:",omitempty"` + User string + }{ + Target: tree.AsStringWithFQNames(&zs, params.Ann()), + Config: strings.TrimSpace(yamlConfig), + Options: optionStr.String(), + User: params.SessionData().User, + } + if deleteZone { + eventLogType = EventLogRemoveZoneConfig } else { - // If the zone config for targetID was a subzone placeholder, it'll have - // been skipped over by GetZoneConfigInTxn. We need to load it regardless - // to avoid blowing away other subzones. - - // TODO(ridwanmsharif): How is this supposed to change? getZoneConfigRaw - // gives no guarantees about completeness. Some work might need to happen - // here to complete the missing fields. The reason is because we don't know - // here if a zone is a placeholder or not. Can we do a GetConfigInTxn here? - // And if it is a placeholder, we use getZoneConfigRaw to create one. - completeZone, err = getZoneConfigRaw(params.ctx, params.p.txn, targetID) - if err != nil { - return err - } else if completeZone == nil { - completeZone = config.NewZoneConfig() - } - completeZone.SetSubzone(config.Subzone{ - IndexID: uint32(index.ID), - PartitionName: partition, - Config: newZone, - }) - - // The partial zone might just be empty. If so, - // replace it with a SubzonePlaceholder. - if subzonePlaceholder { - partialZone.DeleteTableConfig() - } - - partialZone.SetSubzone(config.Subzone{ - IndexID: uint32(index.ID), - PartitionName: partition, - Config: finalZone, - }) + eventLogType = EventLogSetZoneConfig } - - // Finally revalidate everything. Validate only the completeZone config. - if err := completeZone.Validate(); err != nil { - return pgerror.Newf(pgcode.CheckViolation, - "could not validate zone config: %v", err) + return MakeEventLogger(params.extendedEvalCtx.ExecCfg).InsertEventRecord( + params.ctx, + params.p.txn, + eventLogType, + int32(targetID), + int32(params.extendedEvalCtx.NodeID), + info, + ) + } + for _, zs := range specifiers { + // Note(solon): Currently the zone configurations are applied serially for + // each specifier. This could certainly be made more efficient. For + // instance, we should only need to write to the system.zones table once + // rather than once for every specifier. However, the number of specifiers + // is expected to be low--it's bounded by the number of indexes on the + // table--so I'm holding off on adding that complexity unless we find it's + // necessary. + if err := applyZoneConfig(zs); err != nil { + return err } } - - // Write the partial zone configuration. - hasNewSubzones := !deleteZone && index != nil - execConfig := params.extendedEvalCtx.ExecCfg - zoneToWrite := partialZone - - // Finally check for the extra protection partial zone configs would - // require from changes made to parent zones. The extra protections are: - // - // RangeMinBytes and RangeMaxBytes must be set together - // LeasePreferences cannot be set unless Constraints are explicitly set - // Per-replica constraints cannot be set unless num_replicas is explicitly set - if err := zoneToWrite.ValidateTandemFields(); err != nil { - err = errors.Wrap(err, "could not validate zone config") - err = pgerror.WithCandidateCode(err, pgcode.InvalidParameterValue) - err = errors.WithHint(err, - "try ALTER ... CONFIGURE ZONE USING = COPY FROM PARENT [, ...] to populate the field") - return err - } - n.run.numAffected, err = writeZoneConfig(params.ctx, params.p.txn, - targetID, table, zoneToWrite, execConfig, hasNewSubzones) - if err != nil { - return err - } - - // Record that the change has occurred for auditing. - var eventLogType EventLogType - info := struct { - Target string - Config string `json:",omitempty"` - Options string `json:",omitempty"` - User string - }{ - Target: tree.AsStringWithFQNames(&n.zoneSpecifier, params.Ann()), - Config: strings.TrimSpace(yamlConfig), - Options: optionStr.String(), - User: params.SessionData().User, - } - if deleteZone { - eventLogType = EventLogRemoveZoneConfig - } else { - eventLogType = EventLogSetZoneConfig - } - return MakeEventLogger(params.extendedEvalCtx.ExecCfg).InsertEventRecord( - params.ctx, - params.p.txn, - eventLogType, - int32(targetID), - int32(params.extendedEvalCtx.NodeID), - info, - ) + return nil } func (n *setZoneConfigNode) Next(runParams) (bool, error) { return false, nil }