-
Notifications
You must be signed in to change notification settings - Fork 599
Dont use sentinel ids for system tables. #3209
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. It's a simpler and less invasive change than I was afraid of, and meaningfully simplifies the bootstrap path.
Did any of the codegen change? Is it planned to? If not, please remove them from the diff.
This will need some amount of testing with existing commitlogs and snapshots from prior to this change. I'd be comfortable with manual testing, but ideally I'd like to just get a commitlog (starting at zero, doesn't need to be long) and a snapshot (with a short commitlog suffix), check them in to our repo, and write an automated test that we can spin them up in memory and then perform a transaction in the reconstructed state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
The suggestion about testing replay also seems good, although I'm unsure about checking in the artefacts (will we remember to update them if we change formats?).
I argue that we want to leave them in place indefinitely to ensure that we don't break old-but-supported commitlog formats. We'd have to remember to add new ones as well, I suppose, but I'm not terribly worried about that. |
Description of Changes
This simplifies system table bootstrapping logic by removing auto-incremented ids for indexes, constraints, and sequences in system tables. By defining them all up front, we avoid the need for
reset_system_table_schemas
, and make it easier to add new things to system tables.This change should not change the initial system table rows written during bootstrapping, and the
load_1_2_*
tests inrelational_db.rs
should verify that by loading data written by a previous version with and without a snapshot.I also added some sequence-related changes. The sequence schema previous included the
allocated
field, which tracked where to start generating new values after a restart. This made some checks awkward, since the 'schema' was changing as rows were inserted. I moved this outside of theschema
, so those should no longer change unless something actually changes the shape of the tables.As part of that, I also changed where we begin generating on startup to be
allocated
instead ofallocated + 1
. The code that generates values always stopped early enough to allow the next restart to useallocated
, so we were just being overly defensive. We also had some issues if sequences wrapped over, or if we had sequences with negative increments. Those were supported by the schema, but didn't really work with the generation/allocation code. That should have better testing now.Expected complexity level and risk
Testing
This has existing tests covering bootstrapping and restoring from a snapshot.