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

systemschema: stop running PostDeserializationChanges when building tables #105444

Merged

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jun 23, 2023

systemschema: add JobInfo to MakeSystemTables helper

This table was missing from the list, and the expected count of tables
was off since we weren't accounting for non-system tenant tables.

The function is only used for testing, so there was no impact.


systemschema: stop running PostDeserializationChanges when building tables

We would like to remove old PostDeserializationChanges that are no
longer needed. In order to do so, we need to stop relying on them to
build system tables.

Instead, now we adjust the hard-coded system table descriptors and the
related helpers so that they create valid descriptors. This needed two
changes:

  • Update the ConstraintID for check constraints.
  • Update the primary index encoding so that it includes stored columns.

Epic: None
Release note: None

@rafiss rafiss requested a review from chengxiong-ruan June 23, 2023 17:13
@rafiss rafiss requested a review from a team as a code owner June 23, 2023 17:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

rafiss added 2 commits June 24, 2023 00:37
This table was missing from the list, and the expected count of tables
was off since we weren't accounting for non-system tenant tables.

The function is only used for testing, so there was no impact.

Release note: None
…ables

We would like to remove old PostDeserializationChanges that are no
longer needed. In order to do so, we need to stop relying on them to
build system tables.

Instead, now we adjust the hard-coded system table descriptors and the
related helpers so that they create valid descriptors. This needed two
changes:
- Update the ConstraintID for check constraints.
- Update the primary index encoding so that it includes stored columns.

Release note: None
@rafiss rafiss force-pushed the no-post-deserialization-system-schema branch from 0a740a2 to d91fdc4 Compare June 24, 2023 04:38
Copy link
Contributor

@Xiang-Gu Xiang-Gu left a comment

Choose a reason for hiding this comment

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

It looks good to me. One question I have is do we actually have any test to ensure those hard-coded system tables match some certain expected value? This is pertinent to commit 2 where we've changed some that hardcoding. If so, feel free to merge it.

@rafiss
Copy link
Collaborator Author

rafiss commented Jun 26, 2023

One question I have is do we actually have any test to ensure those hard-coded system tables match some certain expected value?

@Xiang-Gu good question, yes we do. It is TestSystemTableLiterals, which is the test that I modified in the first commit. It compares the hard-coded descriptor to what would be created if you executed the CREATE TABLE statement through normal means.

bors r=Xiang-Gu

@craig craig bot merged commit 15d43bb into cockroachdb:master Jun 26, 2023
@craig
Copy link
Contributor

craig bot commented Jun 26, 2023

Build succeeded:

@rafiss rafiss deleted the no-post-deserialization-system-schema branch June 26, 2023 17:22
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.

3 participants