Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: fix multiple UX shortcomings with zone configs #36868

Merged
merged 1 commit into from Apr 16, 2019

Conversation

Projects
5 participants
@knz
Copy link
Member

commented Apr 16, 2019

Fixes #35756.

This patch resolves several separate but related SQL zone config bugs.

  1. SHOW ZONE CONFIG would print out nonsensical ALTER statements
    for inherited database zone configs or the default zone config.

    This was happening because of a conjunction of two implementations
    shortcuts:

    • (*ZoneSpecifier).Format() would print out the "PARTITION"
      keyword as soon as the partition name was set, even for zone
      specifiers for ranges or databases (only tables or indexes can
      have partitions). This is nonsensical.

    • the code for SHOW ZONE CONFIG would determine a zone specifier
      to use for the output ALTER syntax by filling fields from
      the resolved zone details on top of the initial zone specifier.
      So when doing SHOW ZONE CONFIG FOR PARTITION ..., and
      there was no zone config for that partition (so that a database
      or range config would apply), the resulting zone specifier
      would still have its "Partition" field populated.

    This patch fixes the issue by both omitting the PARTITION clause
    for non-table, non-index zone specifiers, and erasing the partition
    field in the ALTER zone specifier in SHOW ZONE CONFIG.

  2. parser does not recognize ALTER syntax printed by SHOW ZONE CONFIG.

    When using SHOW ZONE CONFIG on a partitioned index, the output
    ALTER syntax printed by SHOW would say ALTER PARTITION ... OF INDEX tbl@idx CONFIGURE ZONE ....

    However, the SQL grammar did not recognize ALTER PARTITION OF INDEX, resulting in poor UX. This patch adds support for the
    missing syntax.

  3. SHOW ZONE CONFIG would accept but ignore a partition name
    specifier when using the syntax variant ... FOR TABLE ... PARTITION .... (The partition name was properly handled when
    using the variant ... FOR PARTITION ... OF TABLE ...).

    This was causing suprising/incorrect results when using that
    syntax (especially when combined with the first two bugs).

    This patch ensures that the partition name is not ignored in this form.

  4. SHOW ZONE CONFIG would accept two (equivalent) syntax forms for
    partitioned tables, but just one for indexes. This was
    inconsistent.

    This patch ensures that both the forms FOR INDEX ... PARTITION ... and FOR PARTITION ... OF INDEX ... are supported,f or
    consistency.

Release note (bug fix): SHOW ZONE CONFIGURATION does not any more
emit invalid ALTER syntax in its output when displaying the zone
configuration for a table or index partition that is inheriting from
the database or the default configuration.

Release note (enterprise change): It is now possible to alter the zone
configuration for a secondary index partition using the syntax ALTER PARTITION OF INDEX <tablename>@<indexname> CONFIGURE ZONE ....

Release note (bug fix): SHOW ZONE CONFIGURATION FOR TABLE t PARTITION p does not ignore the clause "PARTITION p" any more and properly
displays the zone configuration for that partition instead.

sql: fix multiple UX shortcomings with zone configs
This patch resolves several separate but related SQL zone config bugs.

1. SHOW ZONE CONFIG would print out nonsensical ALTER statements
   for inherited database zone configs or the default zone config.

   This was happening because of a conjunction of two implementations
   shortcuts:

   - `(*ZoneSpecifier).Format()` would print out the "PARTITION"
     keyword as soon as the partition name was set, even for zone
     specifiers for ranges or databases (only tables or indexes can
     have partitions). This is nonsensical.

   - the code for SHOW ZONE CONFIG would determine a zone specifier
     to use for the output ALTER syntax by filling fields from
	 the resolved zone details *on top* of the initial zone specifier.
	 So when doing SHOW ZONE CONFIG FOR PARTITION ..., *and*
	 there was no zone config for that partition (so that a database
	 or range config would apply), the resulting zone specifier
	 would still have its "Partition" field populated.

   This patch fixes the issue by both omitting the PARTITION clause
   for non-table, non-index zone specifiers, and erasing the partition
   field in the ALTER zone specifier in SHOW ZONE CONFIG.

2. parser does not recognize ALTER syntax printed by SHOW ZONE CONFIG.

   When using SHOW ZONE CONFIG on a partitioned index, the output
   ALTER syntax printed by SHOW would say `ALTER PARTITION ... OF
   INDEX tbl@idx  CONFIGURE ZONE ...`.

   However, the SQL grammar did not recognize `ALTER PARTITION OF
   INDEX`, resulting in poor UX. This patch adds support for the
   missing syntax.

3. SHOW ZONE CONFIG would accept but *ignore* a partition name
   specifier when using the syntax variant `... FOR TABLE
   ... PARTITION ...`. (The partition name was properly handled when
   using the variant `... FOR PARTITION ... OF TABLE ...`).

   This was causing suprising/incorrect results when using that
   syntax (especially when combined with the first two bugs).

   This patch ensures that the partition name is not ignored in this form.

4. SHOW ZONE CONFIG would accept two (equivalent) syntax forms for
   partitioned tables, but just one for indexes. This was
   inconsistent.

   This patch ensures that both the forms `FOR INDEX ... PARTITION
   ... ` and `FOR PARTITION ... OF INDEX ...` are supported,f or
   consistency.

Release note (bug fix): `SHOW ZONE CONFIGURATION` does not any more
emit invalid ALTER syntax in its output when displaying the zone
configuration for a table or index partition that is inheriting from
the database or the default configuration.

Release note (enterprise change): It is now possible to alter the zone
configuration for a secondary index partition using the syntax `ALTER
PARTITION OF INDEX <tablename>@<indexname> CONFIGURE ZONE ...`.

Release note (bug fix): `SHOW ZONE CONFIGURATION FOR TABLE t PARTITION
p` does not ignore the clause "PARTITION p" any more and properly
displays the zone configuration for that partition instead.

@knz knz requested a review from mjibson Apr 16, 2019

@knz knz requested review from cockroachdb/sql-ccl-prs as code owners Apr 16, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

This change is Reviewable

@knz

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

cc @jseldess @nvanbenschoten

@jordanlewis I think this should be backported to 19.1 (after the release, for the .1 patch). Do you agree?

@knz

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

cc @nstewart this is what we were talking about

@awoods187

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

cc @andy-kimball bc he looked here recently

@mjibson
Copy link
Member

left a comment

LGTM, with the caveat that I'm not qualified to review zone config or partition specifics.

@knz

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

LGTM, with the caveat that I'm not qualified to review zone config or partition specifics.

Now you are. :)

(AFAIK no one else in the org has up-to-date expertise in this area at this time.)

@knz

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

bors r+

@knz knz added the docs-todo label Apr 16, 2019

craig bot pushed a commit that referenced this pull request Apr 16, 2019

Merge #36868
36868: sql: fix multiple UX shortcomings with zone configs r=knz a=knz

Fixes #35756.

This patch resolves several separate but related SQL zone config bugs.

1. SHOW ZONE CONFIG would print out nonsensical ALTER statements
   for inherited database zone configs or the default zone config.

   This was happening because of a conjunction of two implementations
   shortcuts:

   - `(*ZoneSpecifier).Format()` would print out the "PARTITION"
     keyword as soon as the partition name was set, even for zone
     specifiers for ranges or databases (only tables or indexes can
     have partitions). This is nonsensical.

   - the code for SHOW ZONE CONFIG would determine a zone specifier
     to use for the output ALTER syntax by filling fields from
	 the resolved zone details *on top* of the initial zone specifier.
	 So when doing SHOW ZONE CONFIG FOR PARTITION ..., *and*
	 there was no zone config for that partition (so that a database
	 or range config would apply), the resulting zone specifier
	 would still have its "Partition" field populated.

   This patch fixes the issue by both omitting the PARTITION clause
   for non-table, non-index zone specifiers, and erasing the partition
   field in the ALTER zone specifier in SHOW ZONE CONFIG.

2. parser does not recognize ALTER syntax printed by SHOW ZONE CONFIG.

   When using SHOW ZONE CONFIG on a partitioned index, the output
   ALTER syntax printed by SHOW would say `ALTER PARTITION ... OF
   INDEX tbl@idx  CONFIGURE ZONE ...`.

   However, the SQL grammar did not recognize `ALTER PARTITION OF
   INDEX`, resulting in poor UX. This patch adds support for the
   missing syntax.

3. SHOW ZONE CONFIG would accept but *ignore* a partition name
   specifier when using the syntax variant `... FOR TABLE
   ... PARTITION ...`. (The partition name was properly handled when
   using the variant `... FOR PARTITION ... OF TABLE ...`).

   This was causing suprising/incorrect results when using that
   syntax (especially when combined with the first two bugs).

   This patch ensures that the partition name is not ignored in this form.

4. SHOW ZONE CONFIG would accept two (equivalent) syntax forms for
   partitioned tables, but just one for indexes. This was
   inconsistent.

   This patch ensures that both the forms `FOR INDEX ... PARTITION
   ... ` and `FOR PARTITION ... OF INDEX ...` are supported,f or
   consistency.

Release note (bug fix): `SHOW ZONE CONFIGURATION` does not any more
emit invalid ALTER syntax in its output when displaying the zone
configuration for a table or index partition that is inheriting from
the database or the default configuration.

Release note (enterprise change): It is now possible to alter the zone
configuration for a secondary index partition using the syntax `ALTER
PARTITION OF INDEX <tablename>@<indexname> CONFIGURE ZONE ...`.

Release note (bug fix): `SHOW ZONE CONFIGURATION FOR TABLE t PARTITION
p` does not ignore the clause "PARTITION p" any more and properly
displays the zone configuration for that partition instead.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@jseldess

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Thanks, @knz!

@craig

This comment has been minimized.

Copy link

commented Apr 16, 2019

Build succeeded

@craig craig bot merged commit 9802577 into cockroachdb:master Apr 16, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@knz knz deleted the knz:20190416-zoneconf branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.