build_sequence_state: clobber pre-existing in-memory Sequences
#2760
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #2758 . Prior to this commit, system sequences (as in, sequences for columns of system tables) were not being correctly re-initialized after replaying from a commitlog. The reproducer for this was to delete the zero snapshot and then replay, and then to advance the sequence by publishing a new module version with a new table. The difference which hid this bug was that, specifically when replaying from nothing, we begin by calling `Locking::bootstrap`, which will `build_sequence_state`, and then after replaying the commitlog, we `rebuild_state_after_replay`, which will `build_sequence_state` again. When replaying from a snapshot, we bypass `bootstrap`, and so the only call to `build_sequence_state` is in `rebuild_state_after_replay` after replaying the suffix of the commitlog. The problem was that, prior to this commit, `build_sequence_state` did not fully overwrite the in-memory sequence state. If an in-memory `Sequence` already existed in the `SequencesState`, `build_sequence_state` would reset that sequence's `value`, but not its `allocation`. Such a pre-existing in-memory `Sequence` could only exist (1) for system sequences and (2) when replaying without a snapshot. If either of these conditions were not met, no `Sequence` object would yet exist, and so `build_sequence_state` would install all of the values from the on-disk state. The fix is simple: unconditionally clobber the in-memory `Sequence` state during `build_sequence_state`, always using the values from the committed state.
kim
approved these changes
May 20, 2025
I also ran this same test on master, and it failed there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Changes
Fixes #2758 .
Prior to this commit, system sequences (as in, sequences for columns of system tables) were not being correctly re-initialized after replaying from a commitlog. The reproducer for this was to delete the zero snapshot and then replay, and then to advance the sequence by publishing a new module version with a new table.
The difference which hid this bug was that, specifically when replaying from nothing, we begin by calling
Locking::bootstrap, which willbuild_sequence_state, and then after replaying the commitlog,we
rebuild_state_after_replay, which willbuild_sequence_stateagain. When replaying from a snapshot, we bypassbootstrap, and so the only call tobuild_sequence_stateis inrebuild_state_after_replayafter replaying the suffix of the commitlog.The problem was that, prior to this commit,
build_sequence_statedid not fully overwrite the in-memory sequence state. If an in-memorySequencealready existed in theSequencesState,build_sequence_statewould reset that sequence'svalue, but not itsallocation. Such a pre-existing in-memorySequencecould only exist (1) for system sequences and (2) when replaying without a snapshot.If either of these conditions were not met, no
Sequenceobject would yet exist, and sobuild_sequence_statewould install all of the values from the on-disk state.The fix is simple: unconditionally clobber the in-memory
Sequencestate duringbuild_sequence_state, always using the values from the committed state.API and ABI breaking changes
N/a
Expected complexity level and risk
2-ish? Sequences and replay are tricky, but I am very confident in this change; using the committed sequence state when building the in-memory sequence state seems obviously correct.
Testing
repro_2758_create_table_after_replay_without_snapshot, which fails on master but passes on this branch.