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

multiregionccl: add unsafe_optimize_system_database #92395

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

jeffswenson
Copy link
Collaborator

@jeffswenson jeffswenson commented Nov 23, 2022

crdb_internal.unsafe_optimize_system_database optimizes the system
database for multi region deployments by configuring latency critical
tables as global or regional by row. Tables that are sensitive to read
latency were converted to global. Table that are sensitive to write
latency were converted to regional by row.

system.sqlliveness and system.sql_instances were reworked by (#90408)
and (#92010) to have a primary key index that is binary compatible with
regional by row tables. The conversion to regional by row is
implemented as a built in function because it is not possible to use
ALTER TABLE ... SET LOCALITY REGIONAL BY ROW to perform the conversion.
Using alter table to convert the RBR tables has two challenges:

  1. There is no way to convert the crdb_region column from []byte to the
    region enum type.
  2. Changing the locality to RBR always rewrites the primary index. The
    sqlliveness and sql_instance subsystems use the raw kv API and expect a
    specific index ID. Rewriting the primary index changes the ID and breaks
    the subsystems.

Release note: None

Part of #85736

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jeffswenson
Copy link
Collaborator Author

@ajwerner this needs more testing before I'm ready to submit it, but can you take a look at let me know what you think of the overall approach?

@jaylim-crl
Copy link
Collaborator

Changing the crdb_region column to an enum type is OK, but the behavior doesn't seem right. Once the type has been changed, we couldn't compare with a string anymore:

root@localhost:26257/defaultdb> select * from system.sql_instances where crdb_region = 'gcp-us-central1';
ERROR: unsupported comparison: bytes to crdb_internal_region
SQLSTATE: 42804

On the other hand, for a regular regional by row table, I could still filter by crdb_region, and the DB knows what the possible values are:

[email protected]:26257/test> select * from users where crdb_region = 'us-central1';
ERROR: invalid input value for enum crdb_internal_region: "us-central1"
SQLSTATE: 22P02
[email protected]:26257/test> select * from users where crdb_region = 'gcp-us-central1';
                   id                  |  city   | first_name | last_name | address
---------------------------------------+---------+------------+-----------+----------
  6f0be85d-4d46-48a5-af3d-b3925b7aaa54 | chicago | foo        | bar       | baz
(1 row)

@ajwerner
Copy link
Contributor

Yes, this approach is sound

@jeffswenson
Copy link
Collaborator Author

Changing the crdb_region column to an enum type is OK, but the behavior doesn't seem right. Once the type has been changed, we couldn't compare with a string anymore:

root@localhost:26257/defaultdb> select * from system.sql_instances where crdb_region = 'gcp-us-central1';
ERROR: unsupported comparison: bytes to crdb_internal_region
SQLSTATE: 42804

On the other hand, for a regular regional by row table, I could still filter by crdb_region, and the DB knows what the possible values are:

[email protected]:26257/test> select * from users where crdb_region = 'us-central1';
ERROR: invalid input value for enum crdb_internal_region: "us-central1"
SQLSTATE: 22P02
[email protected]:26257/test> select * from users where crdb_region = 'gcp-us-central1';
                   id                  |  city   | first_name | last_name | address
---------------------------------------+---------+------------+-----------+----------
  6f0be85d-4d46-48a5-af3d-b3925b7aaa54 | chicago | foo        | bar       | baz
(1 row)

Andrew and I debugged this. The error is coming from the optimizer. The statistics for the table contain the type of the column and that type is used when planning the query. Regenerating statistics for the table causes the error to go away. Arguably we are breaking an invariant in the optimizer: the type of a column is changing, but the id of the column is not changing. For now we will work around this by clearing stats in the unsafe_optimize_system_database built in. During tenant creation, erasing stats will be a no-op, since stats will not have been generated by the time the tenant is created.

@jeffswenson jeffswenson force-pushed the jeffswenson-mr-init branch 3 times, most recently from b3a1cf7 to 9ed141a Compare December 5, 2022 22:26
@jeffswenson jeffswenson marked this pull request as ready for review December 5, 2022 22:29
@jeffswenson jeffswenson requested a review from a team as a code owner December 5, 2022 22:29
@jeffswenson jeffswenson requested a review from a team December 5, 2022 22:29
@jeffswenson
Copy link
Collaborator Author

I removed the draft label. This change is ready for review.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 7 of 8 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jaylim-crl)

crdb_internal.unsafe_optimize_system_database optimizes the system
database for multi region deployments by configuring latency critical
tables as global or regional by row. Tables that are sensitive to read
latency were converted to global. Table that are sensitive to write
latency were converted to regional by row.

system.sqlliveness and system.sql_instances were reworked by (cockroachdb#90408)
and (cockroachdb#92010) to have a primary key index that is binary compatible with
regional by row tables. The conversion to regional by row is
implemented as a built in function because it is not possible to use
`ALTER TABLE ... SET LOCALITY REGIONAL BY ROW` to perform the conversion.
Using alter table to convert the RBR tables has two challenges:
1. There is no way to convert the crdb_region column from []byte to the
   region enum type.
2. Changing the locality to RBR always rewrites the primary index. The
   sqlliveness and sql_instance subsystems use the raw kv API and expect a
   specific index ID. Rewriting the primary index changes the ID and breaks
   the subsystems.

Release note: None

Part of cockroachdb#85736
@jeffswenson
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 6, 2022

Build succeeded:

@craig craig bot merged commit a62d449 into cockroachdb:master Dec 6, 2022
craig bot pushed a commit that referenced this pull request Dec 9, 2022
92588: lease,systemschema: hook up the lease table to regional-by-row partitioning r=ajwerner a=ajwerner

This PR extends the `system.lease` table with REGIONAL BY ROW partitioning for use with #92395. The PR is itself freestanding. 

Epic: CRDB-18596

Release note: None

Co-authored-by: Andrew Werner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants