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: clone pointSynthesizingIter seek keys #90702

Merged

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Oct 26, 2022

The seek keys passed to pointSynthesizingIter seek methods may have originated from UnsafeKey(). Repositioning the parent iterator may therefore invalidate the seek key, but the code keeps using it afterwards.

This patch clones the seek key in a reusable buffer. The overhead of this should be negligible, considering the overall cost of a seek. MVCCGet benchmarks don't show a significant difference:

name                                                                  old time/op    new time/op    delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=0-24     5.11µs ± 1%    5.13µs ± 1%    ~     (p=0.143 n=10+10)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=1-24     9.32µs ± 1%    9.41µs ± 1%  +0.96%  (p=0.001 n=10+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=0-24    19.1µs ± 2%    19.2µs ± 3%    ~     (p=0.075 n=10+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=1-24    25.7µs ± 2%    25.7µs ± 2%    ~     (p=0.853 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24      3.10µs ± 2%    3.09µs ± 1%    ~     (p=0.985 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24      5.05µs ± 0%    5.02µs ± 0%  -0.46%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=0-24     16.5µs ± 3%    16.4µs ± 2%    ~     (p=0.579 n=10+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=1-24     20.6µs ± 2%    20.7µs ± 1%  +0.60%  (p=0.022 n=9+10)

Touches #90642.
Epic: CRDB-2624.

Release note: None

@erikgrinaker erikgrinaker requested review from jbowens and tbg October 26, 2022 14:20
@erikgrinaker erikgrinaker requested a review from a team as a code owner October 26, 2022 14:20
@erikgrinaker erikgrinaker self-assigned this Oct 26, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@erikgrinaker
Copy link
Contributor Author

I'm not really sure this is strictly necessary, because I see that intentInterleavingIter is vulnerable to the same issue, and pebbleMVCCScanner appears to take care to clone the seek key itself for the same reason. That said, pebbleIterator does clone the seek key though, possibly unnecessarily.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Oct 28, 2022

Another site that seems very likely to be buggy:

if ok, err := iter.updateIter(); ok && err == nil {
iter.updateSeekGEPosition(parent.UnsafeKey())
}

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.

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

@erikgrinaker erikgrinaker force-pushed the point-synth-iter-clone-seekkey branch from 60dde93 to 1e66132 Compare October 29, 2022 14:50
The seek keys passed to `pointSynthesizingIter` seek methods may have
originated from `UnsafeKey()`. Repositioning the parent iterator may
therefore invalidate the seek key, but the code keeps using it
afterwards.

This patch clones the seek key in a reusable buffer. The overhead of
this should be negligible, considering the overall cost of a seek.
`MVCCGet` benchmarks don't show a significant difference:

```
name                                                                  old time/op    new time/op    delta
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=0-24     5.11µs ± 1%    5.13µs ± 1%    ~     (p=0.143 n=10+10)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8/numRangeKeys=1-24     9.32µs ± 1%    9.41µs ± 1%  +0.96%  (p=0.001 n=10+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=0-24    19.1µs ± 2%    19.2µs ± 3%    ~     (p=0.075 n=10+10)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8/numRangeKeys=1-24    25.7µs ± 2%    25.7µs ± 2%    ~     (p=0.853 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24      3.10µs ± 2%    3.09µs ± 1%    ~     (p=0.985 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24      5.05µs ± 0%    5.02µs ± 0%  -0.46%  (p=0.000 n=10+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=0-24     16.5µs ± 3%    16.4µs ± 2%    ~     (p=0.579 n=10+10)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8/numRangeKeys=1-24     20.6µs ± 2%    20.7µs ± 1%  +0.60%  (p=0.022 n=9+10)
```

Release note: None
@erikgrinaker
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 30, 2022

Build succeeded:

@craig craig bot merged commit b70d9dd into cockroachdb:master Oct 30, 2022
@erikgrinaker erikgrinaker deleted the point-synth-iter-clone-seekkey branch October 30, 2022 13:08
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.

3 participants