Skip to content
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

storage: disable pebbleIterator range key block filtering #86445

Merged

Conversation

erikgrinaker
Copy link
Contributor

storage: disable pebbleIterator range key block filtering

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)

Resolves #86260.

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

storage: prevent MVCCIncrementalIterator TBI stuck on range key

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

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
@erikgrinaker erikgrinaker requested review from jbowens, tbg and a team August 19, 2022 12:42
@erikgrinaker erikgrinaker self-assigned this Aug 19, 2022
@erikgrinaker erikgrinaker requested a review from a team as a code owner August 19, 2022 12:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

@erikgrinaker
Copy link
Contributor Author

TFTR!

bors r=jbowens

@craig
Copy link
Contributor

craig bot commented Aug 19, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 19, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 19, 2022

Build succeeded:

@craig craig bot merged commit fe19f5d into cockroachdb:master Aug 19, 2022
@erikgrinaker erikgrinaker deleted the disable-range-key-block-filters branch August 23, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: MVCC range keys sometimes omitted by MVCCIncrementalIterator
3 participants