release-22.2: storage: fix point synthesis activation during reverse scans #91028
Conversation
This file contains 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
|
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
|
This change is |
erikgrinaker
force-pushed
the
backport22.2-90899-90953
branch
2 times, most recently
from
dbab539 to
b93816a
Compare
Release note: None
Release note: None
This patch fixes a bug where `pointSynthesizingIter` could appear to
have skipped a synthetic point tombstone in the reverse direction when
enabled dynamically by `pebbleMVCCScanner`.
Consider the following case:
```
REAL DATASET SYNTHETIC DATASET
3 [b3] 3 [b3]
2 a2 2 a2
1 [---) 1 x
a b a b
```
Recall that `pebbleMVCCScanner` only enables `pointSynthesizingIter`
when it encounters a range key. In the case above, during a reverse
scan, the `[a-b)@1` range key will first become visible to
`pebbleMVCCScanner` when it lands on `a@2`, so it enabled point
synthesis positioned at the `a@2` point key. Notice how the iterator has
now skipped over the synthetic point tombstone `a@1`.
This is particularly problematic when combined with `pebbleMVCCScanner`
peeking, which assumes that following a `iterPeekPrev()` call, an
`iterNext()` call can step the parent iterator forward once to get back
to the original position. With the above bug, that is no longer true,
as it instead lands on the synthetic point tombstone which was skipped
during reverse iteration. During intent processing for `b@3`, such an
`iterNext()` call is expected to land on the intent's provisional value
at `b@3`, but it instead lands on the intent itself at `b@0`. This in
turn caused a value checksum or decoding failure, where it was expecting
the current key to be `b@3`, but the actual key was `b@0`.
This patch fixes the bug by keeping track of the direction of the
previous positioning operation in `pebbleMVCCScanner`, and then
repositioning the `pointSynthesizingIter` as appropriate during dynamic
activation in the reverse direction. This doesn't have a significant
effect on performance (in particular, the point get path is never in
reverse).
Several alternative approaches were attempted but abandoned:
* Don't synthesize points at a range key's start bound. While compelling
for a range of reasons (complexity, performance, etc) this has two
flaws: on a bare range key, the iterator still needs to know which
direction to skip in, and we also need to know the timestamps of bare
range keys for `FailOnMoreRecent` handling.
* Only synthesize a single point at a range key's start bound. This
would solve the problem at hand, but has rather strange semantics.
Longer-term, it may be better to merge range key logic into
`pebbleMVCCScanner` itself -- `pointSynthesizingIter` was intended to
reduce complexity, but it's not clear that the overall complexity is
actually reduced.
Release note: None
erikgrinaker
force-pushed
the
backport22.2-90899-90953
branch
from
b93816a to
ffd1485
Compare
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.
Backport:
pebbleMVCCScanner.enablePointSynthesis" (storage: handle errors inpebbleMVCCScanner.enablePointSynthesis#90899)Please see individual PRs for details.
/cc @cockroachdb/release
Release justification: fixes correctness issue with MVCC range tombstones during reverse scans.