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: ALTER TABLE ... SET (to/from) LOCALITY REGIONAL BY ROW is bound to fail when running concurrent REGION ADD/DROP operation #64011

Closed
arulajmani opened this issue Apr 21, 2021 · 2 comments · Fixed by #64117
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@arulajmani
Copy link
Collaborator

arulajmani commented Apr 21, 2021

Describe the problem

Consider the following scenario:

setup:
CREATE DATABASE db PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3"
CREATE TABLE db.t() SET LOCALITY REGIONAL;

steps:
Client 1: ALTER TABLE db.t SET LOCALITY REGIONAL BY ROW; <- block in the schema changer. 
Client 2: ALTER DATABASE db ADD "us-east4"; <- let type schema change finalize
Client 1:  Unblock ALTER TABLE schema change; // Bound to fail.

The issue arises from the fact that when we alter to a REGIONAL BY ROW table, we construct the partitioning in the user transaction. As such, the partitioning for the table t will not contain an entry for "us-east4". As this isn't refreshed in the schema changer, when we try to write the updated table descriptor back, it will fail validation which ensures that every RBR table has a partition for every (PUBLIC) region in the database.

This means that an ALTER TABLE ... SET LOCALITY TO REGIONAL BY ROW is bound to fail if a REGION is added after the schema change is queued up but before it completes. As altering to a REGIONAL BY ROW table involves backfilling indexes, this window is non-trivial.

The problem also exists if we replace the ADD REGION operation with a DROP REGION operation albeit for different reasons. This fails in the schema changer when we try to apply zone configurations on the new index -- as the region has been dropped, the enum value used in the partitioning is no longer present, which results in an error. Normally, partitioning would be negotiated when the REGION DROP is finalized. However, because the table's locality is set to REGIONAL BY ROW only after the schema change completes, this doesn't happen.

** Describe another problem **

setup:
CREATE DATABASE db PRIMARY REGION "us-east1" REGIONS "us-east2", "us-east3"
CREATE TABLE db.t() SET LOCALITY REGIONAL BY ROW;

steps:
Client 1: ALTER TABLE db.t SET GLOBAL <- block in the schema changer. 
Client 2: ALTER DATABASE db ADD "us-east4"; <- let type schema change finalize
Client 1:  Unblock ALTER TABLE schema change;

in this case, the new primary index will have the partitions in db.t will have partitions "us-east1" through 4.

This is because we use NonDropIndexes when finalising. We can just use active indexes if we follow through with #63976

Expected behavior
The schema change should succeed.

Describe the solution you'd like
We shouldn't be creating the partitioning in the user transaction. Instead, we should create it in the schema changer, right before we apply the zone configuration to the index. Currently, we apply the zone configuration before backfilling and after the schema change completes as well. We probably want to do the same with the index partitioning, as the list of active regions on the database can change between these two phases/might change when rolling back a failed ALTER from a REGIONAL BY ROW table.

Additional context
Related to #63462

@arulajmani arulajmani added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 21, 2021
@otan otan changed the title sql: ALTER TABLE ... SET LOCALITY REGIONAL BY ROW is bound to fail when running concurrent REGION ADD/DROP operation sql: ALTER TABLE ... SET (to/from) LOCALITY REGIONAL BY ROW is bound to fail when running concurrent REGION ADD/DROP operation Apr 22, 2021
@ajwerner
Copy link
Contributor

Can we rewrite the partitioning at each step?

@arulajmani
Copy link
Collaborator Author

Can we rewrite the partitioning at each step?

Yeah, that would work too -- I don't think we need to remove the partitioning from the user transaction, as long as we re-write the partitioning list at every step things should work. I plan on picking up @otan's prototype #63976 tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants