sql: support wildcard in ALTER PARTITION OF INDEX #39750

solongordon · 2019-08-19T20:00:17Z

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 <partition name> OF INDEX <table name>@*.

cockroach-teamcity · 2019-08-19T20:00:26Z

This change is

solongordon · 2019-08-19T20:01:40Z

cc @awoods187

Open to suggestions on the syntax here. I know one idea floating around is to make this the default behavior for ALTER PARTITION ... OF TABLE but I thought perhaps we should make it explicit that the user is altering multiple partitions at once.

rohany

This looks good to me -- I like having to make it explicit that the user is altering many partitions here. Perhaps the solution is to just deprecate the alter partition of table command.

Reviewed 3 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rohany)

knz

The overall result is good I suppose, but I have a few nits on the form. See comments below.

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @solongordon)

pkg/sql/set_zone_config.go, line 246 at r1 (raw file):

	}

	// If this is an ALTER ALL PARTITIONS statement, we need to find all indexes

You could have this if/else into a separate top-level function getZoneSpecifiers or something.

pkg/sql/set_zone_config.go, line 250 at r1 (raw file):

	// of them.
	var specifiers []tree.ZoneSpecifier
	if table != nil && n.zoneSpecifier.Partition != "" && n.setAll {

is there a .IsPartitionSpecifier() predicate?

pkg/sql/set_zone_config.go, line 262 at r1 (raw file):

	}

	applyZoneConfig := func(zs tree.ZoneSpecifier) error {

You'd make the code more readable / easier to review by extracting this closure into a separate top level function (also making its captured arguments explicit).

pkg/sql/parser/sql.y, line 1132 at r1 (raw file):

//   ALTER TABLE ... PARTITION BY NOTHING
//   ALTER TABLE ... CONFIGURE ZONE <zoneconfig>
//   ALTER PARTITION ... OF TABLE ... CONFIGURE ZONE <zoneconfig>

This is due an update, or...

pkg/sql/parser/sql.y, line 1248 at r1 (raw file):

//   ALTER INDEX ... UNSPLIT ALL
//   ALTER INDEX ... SCATTER [ FROM ( <exprs...> ) TO ( <exprs...> ) ]
//   ALTER PARTITION ... OF INDEX ... CONFIGURE ZONE <zoneconfig>

... this one, depending on what you choose below.

When when you do also please add the corresponding help text tests

pkg/sql/parser/sql.y, line 1435 at r1 (raw file):

    $$.val = s
  }
| ALTER PARTITION partition_name OF TABLE table_name set_zone_config

While you're at it, I'd recommend extracting the ALTER PARTITION rules (both for tables and indexes) into a separate non-terminal alter_zone_partition_stmt

Then split the help texts for ALTER PARTITION (away from alter_table/alter_index into the new one), add the missing ALTER PARTITION error rule and update the help texts accordingly.
In particular the error rule is essential because now we have so much emphasis on geo partitioning users will welcome some specialized contextual help when they make mistakes.

(I am sad that we have alter partition of table where alter table partition would have been fine... In the same way that we do alter table column and not alter column of table. This makes things so much more complicated to document than otherwise. Oh well.)

pkg/sql/parser/sql.y, line 1445 at r1 (raw file):

    $$.val = s
  }
| ALTER ALL PARTITIONS partition_name OF TABLE table_name set_zone_config

Has this syntax been designed by anyone?

"Alter all partitions" means what it says - take all partitions whatever their definition and apply some change to them. It's unrelated to what you are trying to achieve hiere.
Adding the name afterwards does not clarify meaning/intent; it's actually confusing.

"Alter every object containing a partition with a given name" is conceptually different IMHO.

there's a documentation overhead to introducing new top-level statement keywords. So far we had "ALTER TABLE", "ALTER INDEX" (and out of necessity "ALTER RANGE", although it doesn't make me happy) and this would be adding ALTER ALL.

For example consider:
ALTER INDEXES FOR PARTITION partition_name OF TABLE ...

"all indexes" include the PK so you get the meaning that you want
your execution code is modifying index zone configs, so the statement mirrors what it does
you're piggy backing on ALTER INDEX which is something users already know

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @solongordon)

pkg/sql/set_zone_config.go, line 262 at r1 (raw file):

Previously, knz (kena) wrote…

You'd make the code more readable / easier to review by extracting this closure into a separate top level function (also making its captured arguments explicit).

(or method, that looks easier)

solongordon

Thanks as usual for the thorough review. Would appreciate more thoughts on the syntax.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @solongordon)

pkg/sql/set_zone_config.go, line 246 at r1 (raw file):

Previously, knz (kena) wrote…

You could have this if/else into a separate top-level function getZoneSpecifiers or something.

Just tried this but it doesn't seem to help with readability. Let me know if you feel strongly here.

pkg/sql/set_zone_config.go, line 250 at r1 (raw file):

Previously, knz (kena) wrote…

is there a .IsPartitionSpecifier() predicate?

No, but now there is. Done.

pkg/sql/set_zone_config.go, line 262 at r1 (raw file):

Previously, knz (kena) wrote…

(or method, that looks easier)

That was my first instinct, but the method would need to have something like 8 arguments, so it seemed to hurt readability/reviewability rather than help.

pkg/sql/parser/sql.y, line 1435 at r1 (raw file):

Previously, knz (kena) wrote…

While you're at it, I'd recommend extracting the ALTER PARTITION rules (both for tables and indexes) into a separate non-terminal alter_zone_partition_stmt

Then split the help texts for ALTER PARTITION (away from alter_table/alter_index into the new one), add the missing ALTER PARTITION error rule and update the help texts accordingly.
In particular the error rule is essential because now we have so much emphasis on geo partitioning users will welcome some specialized contextual help when they make mistakes.

(I am sad that we have alter partition of table where alter table partition would have been fine... In the same way that we do alter table column and not alter column of table. This makes things so much more complicated to document than otherwise. Oh well.)

OK, I'll give this a shot.

pkg/sql/parser/sql.y, line 1445 at r1 (raw file):

Previously, knz (kena) wrote…

Has this syntax been designed by anyone?

"Alter all partitions" means what it says - take all partitions whatever their definition and apply some change to them. It's unrelated to what you are trying to achieve hiere.
Adding the name afterwards does not clarify meaning/intent; it's actually confusing.

"Alter every object containing a partition with a given name" is conceptually different IMHO.

there's a documentation overhead to introducing new top-level statement keywords. So far we had "ALTER TABLE", "ALTER INDEX" (and out of necessity "ALTER RANGE", although it doesn't make me happy) and this would be adding ALTER ALL.

For example consider:
ALTER INDEXES FOR PARTITION partition_name OF TABLE ...

"all indexes" include the PK so you get the meaning that you want

your execution code is modifying index zone configs, so the statement mirrors what it does

you're piggy backing on ALTER INDEX which is something users already know

No, the syntax wasn't designed by anyone so I'm happy for the feedback. I agree it's not ideal and take your point about not adding a new ALTER ALL top-level statement.

ALTER INDEXES FOR PARTITION strikes me as confusing though. The current syntax for applying a zone config to a single partition is ALTER PARTITION partition_name OF INDEX .... This implies that a partition belongs to an index. ALTER INDEXES FOR PARTITION makes it sound like it's the other way around: there is a single partition which multiple indexes belong to.

What about just dropping the ALL and using ALTER PARTITIONS partition_name OF TABLE? Or we could just change ALTER PARTITION partition_name OF TABLE to behave this way. (Currently it just modifies the primary index, which is also confusing.) Though as I mentioned earlier, this would hide the fact that multiple partitions are being altered.

I'm starting to lean towards that last option, but none of these feels ideal to me, so I'm certainly open to other ideas if you have them.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @solongordon)

pkg/sql/set_zone_config.go, line 250 at r1 (raw file):

Previously, solongordon (Solon) wrote…

No, but now there is. Done.

Maybe you forgot to commit/push something?

pkg/sql/parser/sql.y, line 1445 at r1 (raw file):

Previously, solongordon (Solon) wrote…

No, the syntax wasn't designed by anyone so I'm happy for the feedback. I agree it's not ideal and take your point about not adding a new ALTER ALL top-level statement.

ALTER INDEXES FOR PARTITION strikes me as confusing though. The current syntax for applying a zone config to a single partition is ALTER PARTITION partition_name OF INDEX .... This implies that a partition belongs to an index. ALTER INDEXES FOR PARTITION makes it sound like it's the other way around: there is a single partition which multiple indexes belong to.

What about just dropping the ALL and using ALTER PARTITIONS partition_name OF TABLE? Or we could just change ALTER PARTITION partition_name OF TABLE to behave this way. (Currently it just modifies the primary index, which is also confusing.) Though as I mentioned earlier, this would hide the fact that multiple partitions are being altered.

I'm starting to lean towards that last option, but none of these feels ideal to me, so I'm certainly open to other ideas if you have them.

Clarifying questions:

if the new thing updates just the primary index what's the difference between alter partitions ... of table and alter partition ... of table ?
Also what's the difference (in general, not just for this new stmt) between of table ... and of index ...@primary?
how is it possible for a table to have multiple partitions that have the same name? Can you provide an example?

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @solongordon)

pkg/sql/parser/sql.y, line 1445 at r1 (raw file):

Previously, knz (kena) wrote…

Clarifying questions:

if the new thing updates just the primary index what's the difference between alter partitions ... of table and alter partition ... of table ?

Also what's the difference (in general, not just for this new stmt) between of table ... and of index ...@primary?

how is it possible for a table to have multiple partitions that have the same name? Can you provide an example?

The new thing actually updates all of the partitions with the provided name across a table's primary and secondary indexes. So as it stands in my first draft, ALTER PARTITION ... OF TABLE would only modify a single partition on the primary index. ALTER ALL PARTITIONS ... OF TABLE might modify multiple partitions across the table's primary and secondary indexes.
ALTER PARTITION ... OF TABLE t and ALTER PARTITION ... OF INDEX t@primary are currently equivalent.
A table and its secondary indexes are all partitioned separately. We recently relaxed a restriction which said a partition name must be unique across all the indexes of a table, since this was a pain point for users. Now it only needs to be unique per index. For example, here is a table with a secondary index which both use the same partition name:

CREATE TABLE t (
    x INT PRIMARY KEY,
    y INT,
    INDEX (y) PARTITION BY LIST (y) (
        PARTITION a VALUES IN (1)
    )
) PARTITION BY LIST (x) (
    PARTITION a VALUES IN (1)
)

knz · 2019-08-21T19:06:15Z

Then what about

ALTER PARTITION foo OF INDEX tbl@*

(note the *)

solongordon · 2019-08-21T19:11:57Z

Ooh, I like it! Maybe a little weird that the tbl@* syntax will be unique to this command, but doesn't bother me too much.

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz)

pkg/sql/set_zone_config.go, line 250 at r1 (raw file):

Previously, knz (kena) wrote…

Maybe you forgot to commit/push something?

Done.

pkg/sql/parser/sql.y, line 1132 at r1 (raw file):

Previously, knz (kena) wrote…

This is due an update, or...

Done.

pkg/sql/parser/sql.y, line 1248 at r1 (raw file):

Previously, knz (kena) wrote…

... this one, depending on what you choose below.

When when you do also please add the corresponding help text tests

Done.

pkg/sql/parser/sql.y, line 1435 at r1 (raw file):

Previously, solongordon (Solon) wrote…

OK, I'll give this a shot.

Done. Please check my work. :)

pkg/sql/parser/sql.y, line 1445 at r1 (raw file):

Previously, solongordon (Solon) wrote…

The new thing actually updates all of the partitions with the provided name across a table's primary and secondary indexes. So as it stands in my first draft, ALTER PARTITION ... OF TABLE would only modify a single partition on the primary index. ALTER ALL PARTITIONS ... OF TABLE might modify multiple partitions across the table's primary and secondary indexes.

ALTER PARTITION ... OF TABLE t and ALTER PARTITION ... OF INDEX t@primary are currently equivalent.

A table and its secondary indexes are all partitioned separately. We recently relaxed a restriction which said a partition name must be unique across all the indexes of a table, since this was a pain point for users. Now it only needs to be unique per index. For example, here is a table with a secondary index which both use the same partition name:
CREATE TABLE t (
    x INT PRIMARY KEY,
    y INT,
    INDEX (y) PARTITION BY LIST (y) (
        PARTITION a VALUES IN (1)
    )
) PARTITION BY LIST (x) (
    PARTITION a VALUES IN (1)
)

Changed to ALTER PARTITION ... OF TABLE tbl@*.

solongordon · 2019-08-26T14:46:29Z

Sorry for the slow turnaround on this. Ready for another look.

knz

Reviewed 9 of 9 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

knz · 2019-08-26T16:41:57Z

nice.

`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 cockroachdb#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 <partition name> OF INDEX <table name>@*`.

solongordon · 2019-08-26T20:18:14Z

TFTR!

bors r+

39750: sql: support wildcard in ALTER PARTITION OF INDEX r=solongordon a=solongordon 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 <partition name> OF INDEX <table name>@*`. Co-authored-by: Solon Gordon <solon@cockroachlabs.com>

craig · 2019-08-26T20:42:38Z

Build succeeded

GitHub CI (Cockroach)

solongordon requested review from knz, rohany and a team August 19, 2019 20:00

rohany approved these changes Aug 20, 2019

View reviewed changes

knz reviewed Aug 20, 2019

View reviewed changes

solongordon commented Aug 21, 2019

View reviewed changes

knz reviewed Aug 21, 2019

View reviewed changes

solongordon commented Aug 21, 2019

View reviewed changes

solongordon force-pushed the alter-partitions branch from 3a53df8 to da1be84 Compare August 26, 2019 14:42

solongordon commented Aug 26, 2019

View reviewed changes

solongordon changed the title ~~sql: add ALTER ALL PARTITIONS command~~ sql: support wildcard in ALTER PARTITION OF INDEX Aug 26, 2019

knz approved these changes Aug 26, 2019

View reviewed changes

solongordon force-pushed the alter-partitions branch from da1be84 to 27e85d1 Compare August 26, 2019 16:54

solongordon force-pushed the alter-partitions branch from 27e85d1 to 12a33d7 Compare August 26, 2019 17:36

craig bot merged commit 12a33d7 into cockroachdb:master Aug 26, 2019

solongordon deleted the alter-partitions branch August 27, 2019 11:59

awoods187 mentioned this pull request Aug 30, 2019

sql: facilitate applying zone configs to same-named partitions #39357

Closed

knz mentioned this pull request Nov 10, 2019

User-facing changes in 19.2 that were not picked up in release notes cockroachdb/docs#5819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: support wildcard in ALTER PARTITION OF INDEX #39750

sql: support wildcard in ALTER PARTITION OF INDEX #39750

solongordon commented Aug 19, 2019 •

edited

Loading

cockroach-teamcity commented Aug 19, 2019

solongordon commented Aug 19, 2019 •

edited

Loading

rohany left a comment

knz left a comment

knz left a comment

solongordon left a comment

knz left a comment

solongordon left a comment

knz commented Aug 21, 2019

solongordon commented Aug 21, 2019

solongordon left a comment

solongordon commented Aug 26, 2019

knz left a comment

knz commented Aug 26, 2019

solongordon commented Aug 26, 2019

craig bot commented Aug 26, 2019

sql: support wildcard in ALTER PARTITION OF INDEX #39750

sql: support wildcard in ALTER PARTITION OF INDEX #39750

Conversation

solongordon commented Aug 19, 2019 • edited Loading

cockroach-teamcity commented Aug 19, 2019

solongordon commented Aug 19, 2019 • edited Loading

rohany left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

knz commented Aug 21, 2019

solongordon commented Aug 21, 2019

solongordon left a comment

Choose a reason for hiding this comment

solongordon commented Aug 26, 2019

knz left a comment

Choose a reason for hiding this comment

knz commented Aug 26, 2019

solongordon commented Aug 26, 2019

craig bot commented Aug 26, 2019

Build succeeded

solongordon commented Aug 19, 2019 •

edited

Loading

solongordon commented Aug 19, 2019 •

edited

Loading