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: update error message when zone configs not applied #39477

Closed
awoods187 opened this issue Aug 9, 2019 · 9 comments · Fixed by #40720
Closed

sql: update error message when zone configs not applied #39477

awoods187 opened this issue Aug 9, 2019 · 9 comments · Fixed by #40720
Assignees
Labels
A-partitioning A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.

Comments

@awoods187
Copy link
Contributor

awoods187 commented Aug 9, 2019

I'm partitioning the Movr table. I've setup the partitions as follows:

ALTER TABLE users PARTITION BY LIST (city) ( PARTITION new_york VALUES IN ('new york'), PARTITION chicago VALUES IN ('chicago'), PARTITION seattle VALUES IN ('seattle') );

ALTER TABLE vehicles PARTITION BY LIST (city) ( PARTITION new_york VALUES IN ('new york'), PARTITION chicago VALUES IN ('chicago'), PARTITION seattle VALUES IN ('seattle') );

ALTER INDEX vehicles_auto_index_fk_city_ref_users PARTITION BY LIST (city) ( PARTITION new_york VALUES IN ('new york'), PARTITION chicago VALUES IN ('chicago'), PARTITION seattle VALUES IN ('seattle') );

ALTER TABLE rides PARTITION BY LIST (city) ( PARTITION new_york VALUES IN ('new york'), PARTITION chicago VALUES IN ('chicago'), PARTITION seattle VALUES IN ('seattle') );

ALTER INDEX rides_auto_index_fk_city_ref_users PARTITION BY LIST (city) ( PARTITION new_york VALUES IN ('new york'), PARTITION chicago VALUES IN ('chicago'), PARTITION seattle VALUES IN ('seattle') );

ALTER INDEX rides_auto_index_fk_vehicle_city_ref_vehicles PARTITION BY LIST (vehicle_city) ( PARTITION new_york VALUES IN ('new york'), PARTITION chicago VALUES IN ('chicago'), PARTITION seattle VALUES IN ('seattle') );

This is great because you can now reuse the same partitioning name. However, it causes a new problem when applying zone constraints.

ALTER PARTITION new_york OF TABLE movr.users CONFIGURE ZONE USING constraints='[+region=us-east1]'; ALTER PARTITION chicago OF TABLE movr.users CONFIGURE ZONE USING constraints='[+region=us-central1]'; ALTER PARTITION seattle OF TABLE movr.users CONFIGURE ZONE USING constraints='[+region=us-west1]';

We can show these constraints:

root@10.142.0.78:26257/movr> show partitions from database movr;
  database_name | table_name | partition_name | parent_partition | column_names |                     index_name                      | partition_value |   zone_constraints
+---------------+------------+----------------+------------------+--------------+-----------------------------------------------------+-----------------+-----------------------+
  movr          | rides      | chicago        | NULL             | vehicle_city | rides@rides_auto_index_fk_vehicle_city_ref_vehicles | ('chicago')     | NULL
  movr          | rides      | chicago        | NULL             | city         | rides@rides_auto_index_fk_city_ref_users            | ('chicago')     | NULL
  movr          | rides      | chicago        | NULL             | city         | rides@primary                                       | ('chicago')     | NULL
  movr          | rides      | new_york       | NULL             | city         | rides@primary                                       | ('new york')    | NULL
  movr          | rides      | new_york       | NULL             | vehicle_city | rides@rides_auto_index_fk_vehicle_city_ref_vehicles | ('new york')    | NULL
  movr          | rides      | new_york       | NULL             | city         | rides@rides_auto_index_fk_city_ref_users            | ('new york')    | NULL
  movr          | rides      | seattle        | NULL             | city         | rides@rides_auto_index_fk_city_ref_users            | ('seattle')     | NULL
  movr          | rides      | seattle        | NULL             | city         | rides@primary                                       | ('seattle')     | NULL
  movr          | rides      | seattle        | NULL             | vehicle_city | rides@rides_auto_index_fk_vehicle_city_ref_vehicles | ('seattle')     | NULL
  movr          | users      | chicago        | NULL             | city         | users@primary                                       | ('chicago')     | [+region=us-central1]
  movr          | users      | new_york       | NULL             | city         | users@primary                                       | ('new york')    | [+region=us-east1]
  movr          | users      | seattle        | NULL             | city         | users@primary                                       | ('seattle')     | [+region=us-west1]
  movr          | vehicles   | chicago        | NULL             | city         | vehicles@primary                                    | ('chicago')     | NULL
  movr          | vehicles   | chicago        | NULL             | city         | vehicles@vehicles_auto_index_fk_city_ref_users      | ('chicago')     | NULL
  movr          | vehicles   | new_york       | NULL             | city         | vehicles@primary                                    | ('new york')    | NULL
  movr          | vehicles   | new_york       | NULL             | city         | vehicles@vehicles_auto_index_fk_city_ref_users      | ('new york')    | NULL
  movr          | vehicles   | seattle        | NULL             | city         | vehicles@primary                                    | ('seattle')     | NULL
  movr          | vehicles   | seattle        | NULL             | city         | vehicles@vehicles_auto_index_fk_city_ref_users      | ('seattle')     | NULL
(18 rows)

Time: 615.713865ms

And we can then add them to a table that also has indexes:

ALTER PARTITION new_york OF TABLE movr.vehicles CONFIGURE ZONE USING constraints='[+region=us-east1]'; ALTER PARTITION chicago OF TABLE movr.vehicles CONFIGURE ZONE USING constraints='[+region=us-central1]'; ALTER PARTITION seattle OF TABLE movr.vehicles CONFIGURE ZONE USING constraints='[+region=us-west1]';

Now the show statement:

root@10.142.0.78:26257/movr> show partitions from database movr;
  database_name | table_name | partition_name | parent_partition | column_names |                     index_name                      | partition_value |   zone_constraints
+---------------+------------+----------------+------------------+--------------+-----------------------------------------------------+-----------------+-----------------------+
  movr          | rides      | chicago        | NULL             | vehicle_city | rides@rides_auto_index_fk_vehicle_city_ref_vehicles | ('chicago')     | NULL
  movr          | rides      | chicago        | NULL             | city         | rides@rides_auto_index_fk_city_ref_users            | ('chicago')     | NULL
  movr          | rides      | chicago        | NULL             | city         | rides@primary                                       | ('chicago')     | NULL
  movr          | rides      | new_york       | NULL             | city         | rides@primary                                       | ('new york')    | NULL
  movr          | rides      | new_york       | NULL             | vehicle_city | rides@rides_auto_index_fk_vehicle_city_ref_vehicles | ('new york')    | NULL
  movr          | rides      | new_york       | NULL             | city         | rides@rides_auto_index_fk_city_ref_users            | ('new york')    | NULL
  movr          | rides      | seattle        | NULL             | city         | rides@rides_auto_index_fk_city_ref_users            | ('seattle')     | NULL
  movr          | rides      | seattle        | NULL             | city         | rides@primary                                       | ('seattle')     | NULL
  movr          | rides      | seattle        | NULL             | vehicle_city | rides@rides_auto_index_fk_vehicle_city_ref_vehicles | ('seattle')     | NULL
  movr          | users      | chicago        | NULL             | city         | users@primary                                       | ('chicago')     | [+region=us-central1]
  movr          | users      | new_york       | NULL             | city         | users@primary                                       | ('new york')    | [+region=us-east1]
  movr          | users      | seattle        | NULL             | city         | users@primary                                       | ('seattle')     | [+region=us-west1]
  movr          | vehicles   | chicago        | NULL             | city         | vehicles@primary                                    | ('chicago')     | [+region=us-central1]
  movr          | vehicles   | chicago        | NULL             | city         | vehicles@vehicles_auto_index_fk_city_ref_users      | ('chicago')     | NULL
  movr          | vehicles   | new_york       | NULL             | city         | vehicles@primary                                    | ('new york')    | [+region=us-east1]
  movr          | vehicles   | new_york       | NULL             | city         | vehicles@vehicles_auto_index_fk_city_ref_users      | ('new york')    | NULL
  movr          | vehicles   | seattle        | NULL             | city         | vehicles@primary                                    | ('seattle')     | [+region=us-west1]
  movr          | vehicles   | seattle        | NULL             | city         | vehicles@vehicles_auto_index_fk_city_ref_users      | ('seattle')     | NULL
(18 rows)

But this does not apply to all of the indexes! In fact, there is no way to make the zone configs apply to the indexes because previously we did it to something like new_york_idx and that isn't specified anymore.

@awoods187
Copy link
Contributor Author

You can't even force it onto an index:

root@10.142.0.78:26257/movr ?> ALTER PARTITION new_york OF TABLE movr.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-east1]'; ALTER PARTITION chicago OF TABLE movr.vehicles CONFIGURE ZONE USING constraints='[+region=us-central1]'; ALTER PARTITION seattle OF TABLE movr.vehicles CONFIGURE ZONE USING constraints='[+region=us-west1]';
invalid syntax: statement ignored: at or near "@": syntax error
DETAIL: source SQL:
ALTER PARTITION new_york OF TABLE movr.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-east1]'
                                               ^
HINT: try \h ALTER TABLE

Note, this is related to #39357

@awoods187 awoods187 added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2 Medium-high impact: many users impacted, risks of availability and difficult-to-fix data errors A-partitioning A-sql-execution Relating to SQL execution. labels Aug 9, 2019
@rohany
Copy link
Contributor

rohany commented Aug 13, 2019

This is actually expected behavior i believe, but it's unclear that this should be the outcome based on the syntax that we have provided.

The ALTER PARTITION p OF TABLE t is equivalent to ALTER PARTITION p OF INDEX t@primary.p, meaning that when we say alter partition of table we only really specify anything about the primary index. So to get what you want out of the partitioning you would need to run ALTER PARTITION new_york OF INDEX movr.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-east1]'; ...

I think figuring out a good way of saying "apply zone configuration z to all partitions named p of table t" is part of what @solongordon is working on.

@awoods187
Copy link
Contributor Author

awoods187 commented Aug 13, 2019

I was able to get this to work by running:

ALTER PARTITION new_york OF INDEX movr.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-east1]'; 
ALTER PARTITION chicago OF INDEX movr.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-central1]'; 
ALTER PARTITION seattle OF INDEX movr.vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-west1]';

It looks like this is a change of behavior when looking at movr previously:
https://www.cockroachlabs.com/docs/stable/demo-geo-partitioning.html#step-12-pin-partitions-to-regions

We partitioned the indexes on rides by running:

ALTER INDEX rides_auto_index_fk_city_ref_users PARTITION BY LIST (city) (
    PARTITION new_york_idx1 VALUES IN ('new york'),
    PARTITION chicago_idx1 VALUES IN ('chicago'),
    PARTITION seattle_idx1 VALUES IN ('seattle')
  );

And then zone configs:

ALTER PARTITION new_york_idx1 OF TABLE movr.rides
    CONFIGURE ZONE USING constraints='[+region=us-east1]';
  ALTER PARTITION chicago_idx1 OF TABLE movr.rides
    CONFIGURE ZONE USING constraints='[+region=us-central1]';
  ALTER PARTITION seattle_idx1 OF TABLE movr.rides
    CONFIGURE ZONE USING constraints='[+region=us-west1]';

We never had to say OF INDEX or specify with the index hint (@).

I think we will need to do three things:

@rohany
Copy link
Contributor

rohany commented Aug 13, 2019

I'm wondering if we should do something like this --
ALTER PARTITION p of TABLE t ... applies a zone configuration to all partitions named p of table t.

To adjust an individual partition you must use the ALTER PARTITION p of INDEX t@i syntax.

It wasn't clear to me while learning this why there was different syntax to adjust the primary index's partitions vs a secondary index's.

@awoods187
Copy link
Contributor Author

yeah i like that idea. How would we handle a ALTER PARTITION p of TABLE t that occurs after a ALTER PARTITION p of INDEX t@i syntax. Overwrite it or respect it? We should brainstorm here

@rohany
Copy link
Contributor

rohany commented Aug 16, 2019

I'm a proponent of each overwriting each other -- I think we should not think about alter partition of table as altering the primary key anymore, and just think of it as changing all partitions of the table. For finer grained adjustment of partitions doing alter partition of index should be the way to do it.

@awoods187
Copy link
Contributor Author

Just confirmed that this also works:

ALTER PARTITION new_york OF INDEX vehicles@vehicles_auto_index_fk_city_ref_users CONFIGURE ZONE USING constraints='[+region=us-east,+az=1]';

If you do not apply the index hint, you get this message:
pq: index "vehicles" does not exist

We should expand upon this to say something like: pq: index "vehicles" does not exist. Specify the index using the index hint syntax table_name@index_name

@awoods187
Copy link
Contributor Author

@ericharmeling to make sure you see this for the docs work

@awoods187 awoods187 added S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. and removed S-2 Medium-high impact: many users impacted, risks of availability and difficult-to-fix data errors labels Aug 30, 2019
@ericharmeling
Copy link
Contributor

@awoods187 Good catch. I’ll open a docs issue and get it in next week.

@awoods187 awoods187 changed the title sql: zone configs not applied when table and index has the same partition name sql: update error message when zone configs not applied Aug 30, 2019
craig bot pushed a commit that referenced this issue Sep 12, 2019
40720: sql: add hint for ALTER PARTITION OF INDEX error r=solongordon a=solongordon

If a user accidentally passes a table name (or some other non-existent
index name) to ALTER PARTITION ... OF INDEX, they now get a helpful hint
suggesting that the use the `<tablename>@<indexname>` syntax to specify
the index.

Fixes #39477

Release note: None

Co-authored-by: Solon Gordon <solon@cockroachlabs.com>
@craig craig bot closed this as completed in 1f9c8d6 Sep 12, 2019
ajwerner pushed a commit to ajwerner/cockroach that referenced this issue Sep 13, 2019
If a user accidentally passes a table name (or some other non-existent
index name) to ALTER PARTITION ... OF INDEX, they now get a helpful hint
suggesting that the use the <tablename>@<indexname> syntax to specify
the index.

Fixes cockroachdb#39477

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-partitioning A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants