fix: correct walfile_start_lsn mask that dropped the LSN high byte - #664
Open
jaideeppyne wants to merge 1 commit into
Open
jaideeppyne wants to merge 1 commit into
jaideeppyne wants to merge 1 commit into
Conversation
LSN.walfile_start_lsn should return the start LSN of the WAL segment that contains this LSN, i.e. `self._seg * WAL_SEG_SIZE` (the LSN with its intra-segment offset cleared). It instead computed `self.lsn & 0xFFFFFFFF000000`. That mask is one hex digit pair too short: as well as clearing the low 24 offset bits it clears bits 56-63 of the 64-bit LSN. For any LSN whose log-id has its high byte set (log-id >= 0x01000000) the result no longer equals `_seg * WAL_SEG_SIZE`, disagrees with `_seg`/`from_walfile_name`, and breaks the `walfile_name` round trip (also corrupting next_/previous_walfile_start_lsn, which are used during WAL iteration). Behaviour is unchanged for the LSN range real clusters reach. Derive the segment start from the segment number so it can no longer drift from the other segment arithmetic. Add a regression test covering a structurally valid WAL file name with a high-byte log-id.
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
LSN.walfile_start_lsnshould return the start LSN of the WAL segment containing the LSN —self._seg * WAL_SEG_SIZE. It instead computedself.lsn & 0xFFFFFFFF000000.Bug
That mask is one hex-digit-pair too short. Besides clearing the low 24 intra-segment offset bits, it clears bits 56–63 of the 64-bit LSN. For any LSN whose log-id has its high byte set (log-id ≥
0x01000000), the result no longer equals_seg * WAL_SEG_SIZE, disagrees with_seg/from_walfile_name, and breaks thewalfile_nameround-trip — which also corruptsnext_walfile_start_lsn/previous_walfile_start_lsnused during WAL iteration. Every other method onLSN(_seg,from_walfile_name,walfile_name) handles the full 64-bit LSN; only this one truncates.In practice the divergence only appears at LSNs far beyond what real clusters reach, so this is an invariant-correctness fix rather than a live-impact one; behaviour is unchanged for realistic LSNs.
Fix
Derive the segment start from the segment number (
self._seg * WAL_SEG_SIZE) so it can no longer drift from the rest of the segment arithmetic. Adds a regression test for a structurally valid WAL file name with a high-byte log-id.