storage: disable pebbleIterator range key block filtering#86445
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Aug 19, 2022
Merged
storage: disable pebbleIterator range key block filtering#86445craig[bot] merged 2 commits intocockroachdb:masterfrom
pebbleIterator range key block filtering#86445craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
This patch disables TBI filtering of MVCC range keys blocks via `MinTimestampHint` and `MaxTimestampHint`, due to complications with `MVCCIncrementalIterator`. Specifically, the TBI and regular iterators would have a different view of the range key fragmentation, which could cause it to skip past range key fragments. The necessary seeks and other processing to handle this would likely negate the benefit of the filtering. This causes a minor regression when range keys are present, because the TBI optimization can currently get "stuck" when seeking into range keys. Previously, these range keys would be filtered out by the TBI. A separate commit will address this. ``` name old time/op new time/op delta CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24 804ms ± 1% 798ms ± 1% ~ (p=0.151 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24 990ms ± 1% 972ms ± 1% -1.84% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24 991ms ± 0% 984ms ± 1% -0.73% (p=0.032 n=4+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=0-24 676ms ± 0% 689ms ± 1% +2.03% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=1-24 859ms ± 0% 867ms ± 1% +0.95% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=100-24 868ms ± 0% 877ms ± 0% +1.06% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=0-24 181ms ± 0% 183ms ± 1% +0.90% (p=0.016 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=1-24 204ms ± 1% 206ms ± 1% +1.20% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=100-24 294ms ± 1% 323ms ± 4% +9.87% (p=0.008 n=5+5) ``` Release justification: bug fixes and low-risk updates to new functionality Release note: None
This patch prevents the internal TBI used in `MVCCIncrementalIterator` from getting prematurely stuck on an MVCC range key covering a seek key. Consider the following visible keys for the TBI (time range 3-5) and regular iterator: Iterator: `[a-f)@5`, `a@4`, `c@1`, and `e@4` TBI: `[a-f)@5`, `a@4`, `e@4`. If the iterator is positioned on `c@1` (outside the time bounds), and the TBI is seeked to `c` to find newer keys, the the TBI will get stuck on the range key `[a-f)@5` even though it has already been emitted, preventing the TBI from being used at all. This patch moves the TBI forward to `e@4` in this case, and similarly when seeking iter ahead. The effect on performance appears rather marginal though. ``` name old time/op new time/op delta CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24 798ms ± 1% 805ms ± 1% +0.93% (p=0.032 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24 972ms ± 1% 983ms ± 0% +1.15% (p=0.016 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24 984ms ± 1% 995ms ± 0% +1.09% (p=0.016 n=5+4) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=0-24 689ms ± 1% 681ms ± 0% -1.28% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=1-24 867ms ± 1% 854ms ± 0% -1.57% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=100-24 877ms ± 0% 869ms ± 1% ~ (p=0.056 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=0-24 183ms ± 1% 179ms ± 0% -1.90% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=1-24 206ms ± 1% 204ms ± 1% -1.10% (p=0.016 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=100-24 323ms ± 4% 315ms ± 1% -2.37% (p=0.016 n=5+5) ``` Release justification: bug fixes and low-risk updates to new functionality Release note: None
Member
|
This change is |
jbowens
approved these changes
Aug 19, 2022
Contributor
jbowens
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @tbg)
Contributor
Author
|
TFTR! bors r=jbowens |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
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
3 participants
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.
storage: disable
pebbleIteratorrange key block filteringThis patch disables TBI filtering of MVCC range keys blocks via
MinTimestampHintandMaxTimestampHint, due to complications withMVCCIncrementalIterator. Specifically, the TBI and regular iteratorswould have a different view of the range key fragmentation, which could
cause it to skip past range key fragments. The necessary seeks and other
processing to handle this would likely negate the benefit of the
filtering.
This causes a minor regression when range keys are present, because the
TBI optimization can currently get "stuck" when seeking into range keys.
Previously, these range keys would be filtered out by the TBI. A
separate commit will address this.
Resolves #86260.
Release justification: bug fixes and low-risk updates to new functionality
Release note: None
storage: prevent
MVCCIncrementalIteratorTBI stuck on range keyThis patch prevents the internal TBI used in
MVCCIncrementalIteratorfrom getting prematurely stuck on an MVCC range key covering a seek key.
Consider the following visible keys for the TBI (time range 3-5) and
regular iterator:
Iterator:
[a-f)@5,a@4,c@1, ande@4TBI:
[a-f)@5,a@4,e@4.If the iterator is positioned on
c@1(outside the time bounds), andthe TBI is seeked to
cto find newer keys, the the TBI will get stuckon the range key
[a-f)@5even though it has already been emitted,preventing the TBI from being used at all. This patch moves the TBI
forward to
e@4in this case, and similarly when seeking iter ahead.The effect on performance appears rather marginal though.
Release justification: bug fixes and low-risk updates to new functionality
Release note: None