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: add range key support for MVCCIncrementalIterator #82691

Merged

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Jun 9, 2022

storage: minor MVCCIncrementalIterator cleanup

This patch makes some minor, mechanical cleanups of
MVCCIncrementalIterator:

  • checkValidAndSaveErr was renamed updateValid.

  • initMetaAndCheckForIntentOrInlineError was renamed updateMeta.

  • NextKeyIgnoringTime was removed, as it was never used.

  • Key() and Value() were removed, as they are not part of the
    SimpleMVCCIterator interface.

  • Method comments that were copies of interface method comments were
    replaced with "implements SimpleMVCCIterator" variants.

Release note: None

storage: add range key support for MVCCIncrementalIterator

This patch adds range key support for MVCCIncrementalIterator,
filtering them by the time bounds and exposing them via the usual
SimpleMVCCIterator interface.

This comes with a moderate performance penalty in the no-range-key case,
mostly due to additional HasPointAndRange() checks. This, and the
range key paths, will be optimized later. For now, correctness is
sufficient, in order to unblock higher-level work.

name                                                 old time/op  new time/op  delta
MVCCIncrementalIterator/ts=5-24                      11.7ms ± 9%  12.1ms ± 3%    ~     (p=0.065 n=9+10)
MVCCIncrementalIterator/ts=480-24                     442µs ± 1%   444µs ± 1%    ~     (p=0.094 n=9+9)
MVCCIncrementalIteratorForOldData/valueSize=100-24   1.41ms ± 1%  1.49ms ± 1%  +5.59%  (p=0.000 n=10+10)
MVCCIncrementalIteratorForOldData/valueSize=500-24   1.89ms ± 2%  1.98ms ± 2%  +4.61%  (p=0.000 n=10+10)
MVCCIncrementalIteratorForOldData/valueSize=1000-24  2.59ms ± 2%  2.68ms ± 1%  +3.33%  (p=0.000 n=10+10)
MVCCIncrementalIteratorForOldData/valueSize=2000-24  4.15ms ± 2%  4.12ms ± 3%    ~     (p=0.481 n=10+10)

Release note: None

@erikgrinaker erikgrinaker requested review from itsbilal, nicktrav, jbowens, aliher1911 and a team June 9, 2022 21:18
@erikgrinaker erikgrinaker requested review from a team as code owners June 9, 2022 21:18
@erikgrinaker erikgrinaker self-assigned this Jun 9, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@shermanCRL shermanCRL requested a review from msbutler June 10, 2022 01:58
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-incremental-iter branch from 8a6c900 to c8fb855 Compare June 10, 2022 07:35
Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @itsbilal, @jbowens, @msbutler, and @nicktrav)


pkg/storage/mvcc_incremental_iterator.go line 434 at r2 (raw file):

		if hasRange {
			if rangeStart := i.iter.RangeBounds().Key; !rangeStart.Equal(i.rangeKeysStart) {
				i.rangeKeysStart = append(i.rangeKeysStart[:0], rangeStart...)

@jbowens This pattern of detecting when the range key changes by comparing its start bound to a previous start bound keeps coming up all over the place. It would be nice if we could avoid these constant key comparisons. I'm guessing Pebble has already done this comparison during interleaving, so if we could expose that somehow then we would avoid a bunch of these comparisons higher up the stack -- maybe something as simple as a RangeKeysChanged() method.

This isn't something that needs to be addressed right away, but when we come back around to optimize the range key paths then I suspect this would be fruitful.

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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @itsbilal, @msbutler, and @nicktrav)


pkg/storage/mvcc_incremental_iterator.go line 434 at r2 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

@jbowens This pattern of detecting when the range key changes by comparing its start bound to a previous start bound keeps coming up all over the place. It would be nice if we could avoid these constant key comparisons. I'm guessing Pebble has already done this comparison during interleaving, so if we could expose that somehow then we would avoid a bunch of these comparisons higher up the stack -- maybe something as simple as a RangeKeysChanged() method.

This isn't something that needs to be addressed right away, but when we come back around to optimize the range key paths then I suspect this would be fruitful.

Yeah, definitely. The same problem/dynamic exists between Pebble's internal iterators: A pebble.Iterator sometimes needs to needlessly save a copy of range key state, and we have a TODO to avoid it if the range key didn't change.

I think we'll definitely be able to expose something through the Iterator interface as well.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-incremental-iter branch 2 times, most recently from 7243af1 to 2f5a095 Compare June 13, 2022 13:06
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-incremental-iter branch from 2f5a095 to 428f4ee Compare June 14, 2022 08:51
Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

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

Does it make sense to have a test that checks if other KeyTypes work? PointsOnly is likely covered by existing tests since we pass the value down, but RangesOnly might be interesting.

pkg/storage/mvcc_incremental_iterator.go Outdated Show resolved Hide resolved
@erikgrinaker
Copy link
Contributor Author

Does it make sense to have a test that checks if other KeyTypes work? PointsOnly is likely covered by existing tests since we pass the value down, but RangesOnly might be interesting.

Yeah, good call -- added some tests for PointsOnly and RangesOnly.

Copy link
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

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

💯

I like the iterator simplification bits!

@erikgrinaker
Copy link
Contributor Author

TFTQR!

bors r=aliher1911

@craig
Copy link
Contributor

craig bot commented Jun 16, 2022

Build failed (retrying...):

This patch makes some minor, mechanical cleanups of
`MVCCIncrementalIterator`:

* `checkValidAndSaveErr` was renamed `updateValid`.

* `initMetaAndCheckForIntentOrInlineError` was renamed `updateMeta`.

* `NextKeyIgnoringTime` was removed, as it was never used.

* `Key()` and `Value()` were removed, as they are not part of the
  `SimpleMVCCIterator` interface.

* Method comments that were copies of interface method comments were
  replaced with "implements SimpleMVCCIterator" variants.

Release note: None
@erikgrinaker
Copy link
Contributor Author

bors r-

Test failures due to changes on master.

@craig
Copy link
Contributor

craig bot commented Jun 16, 2022

Canceled.

This patch adds range key support for `MVCCIncrementalIterator`,
filtering them by the time bounds and exposing them via the usual
`SimpleMVCCIterator` interface.

This comes with a moderate performance penalty in the no-range-key case,
mostly due to additional `HasPointAndRange()` checks. This, and the
range key paths, will be optimized later. For now, correctness is
sufficient, in order to unblock higher-level work.

```
name                                                 old time/op  new time/op  delta
MVCCIncrementalIterator/ts=5-24                      11.7ms ± 9%  12.1ms ± 3%    ~     (p=0.065 n=9+10)
MVCCIncrementalIterator/ts=480-24                     442µs ± 1%   444µs ± 1%    ~     (p=0.094 n=9+9)
MVCCIncrementalIteratorForOldData/valueSize=100-24   1.41ms ± 1%  1.49ms ± 1%  +5.59%  (p=0.000 n=10+10)
MVCCIncrementalIteratorForOldData/valueSize=500-24   1.89ms ± 2%  1.98ms ± 2%  +4.61%  (p=0.000 n=10+10)
MVCCIncrementalIteratorForOldData/valueSize=1000-24  2.59ms ± 2%  2.68ms ± 1%  +3.33%  (p=0.000 n=10+10)
MVCCIncrementalIteratorForOldData/valueSize=2000-24  4.15ms ± 2%  4.12ms ± 3%    ~     (p=0.481 n=10+10)
```

Release note: None
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-incremental-iter branch from 0473c73 to 544393e Compare June 16, 2022 16:00
@erikgrinaker
Copy link
Contributor Author

bors retry

@erikgrinaker
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 16, 2022

Build succeeded:

@craig craig bot merged commit 877542d into cockroachdb:master Jun 16, 2022
@erikgrinaker erikgrinaker deleted the mvcc-range-tombstones-incremental-iter branch June 16, 2022 19:36
msbutler added a commit to msbutler/cockroach that referenced this pull request Jul 6, 2022
The MVCCInvermentalIterator's NextKeyIgnoreTime() function was previously
deleted in cockroachdb#82691, as there wasn't any use for it at the time. Now, the new
Delete Range with predicate filtering will need it (cockroachdb#83676).

This PR also cleans up duplicate code used to test `NextIgnoringTime` and
`NextKeyIgnoringTime`.

Release note: None
msbutler added a commit to msbutler/cockroach that referenced this pull request Jul 7, 2022
The MVCCInvermentalIterator's NextKeyIgnoringTime() function was previously
deleted in cockroachdb#82691, as there wasn't any use for it at the time. Now, the new
Delete Range with predicate filtering will need it (cockroachdb#83676).

This PR also cleans up duplicate code used to test NextIgnoringTime and
NextKeyIgnoringTime.

Release note: None
msbutler added a commit to msbutler/cockroach that referenced this pull request Jul 8, 2022
The MVCCInvermentalIterator's NextKeyIgnoringTime() function was previously
deleted in cockroachdb#82691, as there wasn't any use for it at the time. Now, the new
Delete Range with predicate filtering will need it (cockroachdb#83676).

This PR also cleans up duplicate code used to test NextIgnoringTime and
NextKeyIgnoringTime.

Release note: None
craig bot pushed a commit that referenced this pull request Jul 8, 2022
83729: storage: add nextKeyIgnoringTime() to MVCCIncrementalIterator r=erikgrinaker a=msbutler

The MVCCInvermentalIterator's NextKeyIgnoringTime() function was previously
deleted in #82691, as there wasn't any use for it at the time. Now, the new
Delete Range with predicate filtering will need it (#83676).

This PR also cleans up duplicate code used to test NextIgnoringTime and
NextKeyIgnoringTime.

Release note: None

Co-authored-by: Michael Butler <[email protected]>
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.

4 participants