storage: implement RangeKeyChanged() for MVCCIncrementalIterator
#86259
Conversation
|
This change is |
erikgrinaker
changed the title
RangeKeyChanged() for MVCCIncrementalIterator
erikgrinaker
force-pushed
the
mvcc-incremental-iter-rangekeychanged
branch
5 times, most recently
from
0c84cca
to
f56dcfb
Compare
There was a problem hiding this comment.
First pass, still need to get a little more confident with the code, if you put a silly bug in there on purpose I probably would've missed it. Haven't looked at the test output yet. I will work a bit on #86122 to get more comfortable with the harness.
Reviewed 2 of 2 files at r2, 2 of 2 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @jbowens)
-- commits line 51 at r4:
This
pkg/storage/mvcc_history_test.go line 1611 at r4 (raw file):
if e.iterRangeKeys.IsEmpty() { e.iterRangeKeys = MVCCRangeKeyStack{ Bounds: roachpb.Span{Key: keys.MinKey.Next().Clone(), EndKey: keys.MaxKey.Clone()},
Why the clone? Do we need to update keys.MinKey etc so that its cap equals its len?
pkg/storage/mvcc_incremental_iterator.go line 336 at r2 (raw file):
return false } tbiKey = i.timeBoundIter.UnsafeKey().Key
Could i.timeBoundIter still be positioned with (hasRange && !hasPoint) at this point, i.e. can we access UnsafeKey() unconditionally here?
pkg/storage/mvcc_incremental_iterator.go line 434 at r2 (raw file):
} if skipped { // The skip may landed in the middle of a bare range key, in which case we
may have landed
pkg/storage/mvcc_incremental_iterator.go line 435 at r2 (raw file):
if skipped { // The skip may landed in the middle of a bare range key, in which case we // should move on to the next key.
Why is this done here and not in maybeSkipKeys?
pkg/storage/mvcc_incremental_iterator.go line 96 at r3 (raw file):
// underlying iterator returns true as well. This can be used to hide point or // range keys where one key kind satisfies the time predicate but the other // one doesn't. Ignored following IgnoringTime() calls.
Ignored for how long? Consider expanding the comment by referencing i.ignoringTime and expanding its comment a bit to point out when the flag is reset (I guess that's when we're done with the IgnoringTime call). The code isn't so clear; we actually reset ignoringTime at the top of advance, I think someplace should explain how this works and on the field is as good a place as any. Maybe advance should also be renamed to reflect that it doesn't ignore time? advanceObservingTime? You could similarly rename hasPoint and hasRange with an ObservingTime suffix.
pkg/storage/mvcc_incremental_iterator.go line 247 at r3 (raw file):
} i.iter.SeekGE(startKey) i.advance(true /* seek */)
Should the parameter be called seeked, since it reflects what the caller did before? As is, this sounds like imperative mood which doesn't seem right.
edit: with the comment on advance this seems clear enough, still going to submit this for your consideration.
pkg/storage/mvcc_incremental_iterator.go line 480 at r3 (raw file):
// isn't visible either, then we keep going. if !i.hasPoint { if !i.hasRange {
Not new code, but I thought one of (hasPoint,hasRange) them has to be true for a valid iterator. This isn't true here because these values come from updateRangeKeys which filters them by time, so we could have both false. Mind pointing this out.
pkg/storage/mvcc_incremental_iterator.go line 486 at r4 (raw file):
newRangeKey = i.hasRange if !hadRange && !i.hasRange { i.rangeKeyChanged = false
It's false already, so you don't need this?
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
// However, RangeKeyChanged() will only fire if this moves onto an entirely new // MVCC range key, not if it reveals additional range keys at the current // position.
Are you confident that these are the right semantics? Is it what CatchUpScan wants? It feels like this might require awkward code at each caller that also uses *IgnoringTime. Could it be better to trigger an event in this case?
If the current semantics are desirable, maybe a way to phrase them as less of a one-off is that RangeKeyChanged triggers whenever we traverse (i.e. cross or step onto) the start key of a range deletion. This is not the case if we call NextIgnoringTime while already positioned on a hidden range key, so there is no surprise that RangeKeyChanged won't fire.
I am still suspicious that current semantics are the right choice. Callers may use RangeKeyChanged to cache state that depends on the range key. Seems to invite bugs if the range key may change without the corresponding notification firing.
pkg/storage/testdata/mvcc_histories/range_key_iter_incremental line 129 at r4 (raw file):
iter_scan: "a"/4.000000000,0=/<empty> {a-b}/[1.000000000,0=/<empty>] iter_scan: "a"/2.000000000,0=/BYTES/a2 {a-b}/[1.000000000,0=/<empty>] iter_scan: {b-c}/[3.000000000,0=/<empty> 1.000000000,0=/<empty>] !
I don't see code in this diff that produces the exclamation mark.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_history_test.go line 1611 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Why the clone? Do we need to update
keys.MinKeyetc so that its cap equals its len?
Otherwise, when we call CloneInto() later which reuses the byte slices, we're trampling keys.MinKey and keys.MaxKey, which is really bad. I'll add a comment.
pkg/storage/mvcc_incremental_iterator.go line 435 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Why is this done here and not in
maybeSkipKeys?
We need to record and propagate the RangeKeyChanged state during the steps. By doing this here, maybeSkipKeys only ever takes one step, so we can keep all of the RangeKeyChanged logic in one spot. Initially tried returning a value for it instead, but don't think it ended up being as clear -- either works though, no strong opinion.
pkg/storage/mvcc_incremental_iterator.go line 96 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Ignored for how long? Consider expanding the comment by referencing
i.ignoringTimeand expanding its comment a bit to point out when the flag is reset (I guess that's when we're done with the IgnoringTime call). The code isn't so clear; we actually resetignoringTimeat the top ofadvance, I think someplace should explain how this works and on the field is as good a place as any. Maybeadvanceshould also be renamed to reflect that it doesn't ignore time?advanceObservingTime? You could similarly renamehasPointandhasRangewith anObservingTimesuffix.
Until the next positioning operation, which may or may not be another IgnoringTime operation. Will expand.
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Are you confident that these are the right semantics? Is it what
CatchUpScanwants? It feels like this might require awkward code at each caller that also uses*IgnoringTime. Could it be better to trigger an event in this case?If the current semantics are desirable, maybe a way to phrase them as less of a one-off is that RangeKeyChanged triggers whenever we traverse (i.e. cross or step onto) the start key of a range deletion. This is not the case if we call
NextIgnoringTimewhile already positioned on a hidden range key, so there is no surprise that RangeKeyChanged won't fire.I am still suspicious that current semantics are the right choice. Callers may use
RangeKeyChangedto cache state that depends on the range key. Seems to invite bugs if the range key may change without the corresponding notification firing.
I've been going back and forth on this.
The main reason I landed on this is that if the caller is doing any caching, then triggering RangeKeyChanged´ when NextIgnoringTimereveals additional versions is going to ruin that caching anyway -- in the worst case, it's going to flip back and forth for everyNext()/NextIgnoringTime()` cycle, effectively making the caching worse than not caching at all.
Because MVCCIncrementalIterator already has to decode and process the range keys (for filtering), we can cheaply cache both of them in here, and so I think the recommended way to use this would be for the caller to not do any caching at all, but simply call RangeKeys() at the spot they need it and process as needed -- for example, this simplifies the WithDiff processing in catchupscan, which just needs to know if there's a range key between two points regardless of time filtering.
Another option I've been considering has been to always expose filtered range keys in RangeKeys(), and have a separate RangeKeysIgnoringTime() to expose unfiltered range keys. However, in that case it isn't clear what the semantics of RangeKeyChanged should be when RangeKeysIgnoringTime() changes but RangeKeys()doesn't.
Happy to talk through this on a call or something.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
I've been going back and forth on this.
The main reason I landed on this is that if the caller is doing any caching, then triggering
RangeKeyChanged´ whenNextIgnoringTimereveals additional versions is going to ruin that caching anyway -- in the worst case, it's going to flip back and forth for everyNext()/NextIgnoringTime()` cycle, effectively making the caching worse than not caching at all.Because
MVCCIncrementalIteratoralready has to decode and process the range keys (for filtering), we can cheaply cache both of them in here, and so I think the recommended way to use this would be for the caller to not do any caching at all, but simply callRangeKeys()at the spot they need it and process as needed -- for example, this simplifies theWithDiffprocessing in catchupscan, which just needs to know if there's a range key between two points regardless of time filtering.Another option I've been considering has been to always expose filtered range keys in
RangeKeys(), and have a separateRangeKeysIgnoringTime()to expose unfiltered range keys. However, in that case it isn't clear what the semantics ofRangeKeyChangedshould be whenRangeKeysIgnoringTime()changes butRangeKeys()doesn't.Happy to talk through this on a call or something.
How about
RangeKeyChanged()only triggers on filtered range keys, even ifXIgnoringTimeexposes an unseen, out-of-bounds range key.XIgnoringTime() (rangeKeyChanged bool)returns whether there's now an exclusively ignored rangekey, which can be retrieved via:RangeKeyIgnoringTime() MVCCRangeKeyStack
This seems good because:
- caching based on
RangeKeywhen all you care about is filtered ones "just works" - don't randomly expose
RangeKeys that are out of time bounds after a call toXIgnoringKey... - ... but if those are relevant, the event tracking and access to them is there, and
- it's nicely contained in the three methods related to ignoring time bounds.
This is also operating under the assumption that access to out-of-bounds range keys will be somewhat rare. The uses I've seen revolve around discovering a version matching an intent, finding a previous version for withDiff, etc.
Would be interesting to litmus test this with MVCCPredicateDeleteRange and/or MVCCClearRange since both probably do want to see out-of-bound range dels.
This patch uses `RangeKeyChanged()` to detect changes to range keys in `MVCCIncrementalIterator`. There are no functional changes. Some quick benchmarks, using catchup scans: ``` name old time/op new time/op delta CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=0-24 538ms ± 1% 535ms ± 1% ~ (p=0.211 n=10+9) CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=1-24 690ms ± 0% 590ms ± 1% -14.56% (p=0.000 n=9+10) CatchUpScan/mixed-case/withDiff=true/perc=0.00/numRangeKeys=100-24 743ms ± 1% 646ms ± 1% -13.13% (p=0.000 n=9+9) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24 794ms ± 1% 794ms ± 0% ~ (p=0.579 n=10+10) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24 966ms ± 0% 911ms ± 1% -5.72% (p=0.000 n=10+10) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24 974ms ± 0% 920ms ± 0% -5.51% (p=0.000 n=10+10) ``` Release justification: bug fixes and low-risk updates to new functionality. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_incremental_iterator.go line 336 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Could
i.timeBoundIterstill be positioned with (hasRange && !hasPoint) at this point, i.e. can we accessUnsafeKey()unconditionally here?
Yes, UnsafeKey() is also valid along a range key.
pkg/storage/mvcc_incremental_iterator.go line 435 at r2 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
We need to record and propagate the
RangeKeyChangedstate during the steps. By doing this here,maybeSkipKeysonly ever takes one step, so we can keep all of theRangeKeyChangedlogic in one spot. Initially tried returning a value for it instead, but don't think it ended up being as clear -- either works though, no strong opinion.
Ended up moving this back into maybeSkipKeys, it's cleaner.
pkg/storage/mvcc_incremental_iterator.go line 247 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Should the parameter be called
seeked, since it reflects what the caller did before? As is, this sounds like imperative mood which doesn't seem right.edit: with the comment on
advancethis seems clear enough, still going to submit this for your consideration.
Sure, done.
pkg/storage/mvcc_incremental_iterator.go line 486 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
It's false already, so you don't need this?
We need this in the case where we weren't on a range key, then stepped onto a new range key, but it got filtered out. This shouldn't trigger RangeKeyChanged() to the caller, since we're going from empty to empty.
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
How about
RangeKeyChanged()only triggers on filtered range keys, even ifXIgnoringTimeexposes an unseen, out-of-bounds range key.XIgnoringTime() (rangeKeyChanged bool)returns whether there's now an exclusively ignored rangekey, which can be retrieved via:RangeKeyIgnoringTime() MVCCRangeKeyStackThis seems good because:
- caching based on
RangeKeywhen all you care about is filtered ones "just works"- don't randomly expose
RangeKeys that are out of time bounds after a call toXIgnoringKey...- ... but if those are relevant, the event tracking and access to them is there, and
- it's nicely contained in the three methods related to ignoring time bounds.
This is also operating under the assumption that access to out-of-bounds range keys will be somewhat rare. The uses I've seen revolve around discovering a version matching an intent, finding a previous version for withDiff, etc.
Would be interesting to litmus test this withMVCCPredicateDeleteRangeand/orMVCCClearRangesince both probably do want to see out-of-bound range dels.
Yeah, let me do another pass over this. Seems reasonable to stick to filtered range keys by default. We may not even need XIgnoringTime() (rangeKeyChanged bool) at all -- since RangeKeyIgnoringTime() is ~free, the caller can just call it unconditionally when needed.
pkg/storage/testdata/mvcc_histories/range_key_iter_incremental line 129 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
I don't see code in this diff that produces the exclamation mark.
It was added back in #86056, but MVCCIncrementalIterator was explicitly omitted because it didn't implement RangeKeyChanged(). See changes in mvcc_history_test.go which remove those exclusions.
erikgrinaker
force-pushed
the
mvcc-incremental-iter-rangekeychanged
branch
3 times, most recently
from
ea57915
to
8a2f31a
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Yeah, let me do another pass over this. Seems reasonable to stick to filtered range keys by default. We may not even need
XIgnoringTime() (rangeKeyChanged bool)at all -- sinceRangeKeyIgnoringTime()is ~free, the caller can just call it unconditionally when needed.
Done. All standard range key methods now only apply to filtered range keys. Unfiltered range keys are available via RangeKeysIgnoringTime().
The only wart here is that it's now possible for HasPointAndRange() to return false,false if NextIgnoringTime steps onto a bare range key that's outside the time bounds, in violation of the SimpleMVCCIterator contract. This isn't really an issue for the current callers, but it is a bit of a footgun so one has to be careful -- then again, one does have to be careful with the IgnoringTime methods in general.
erikgrinaker
force-pushed
the
mvcc-incremental-iter-rangekeychanged
branch
2 times, most recently
from
fc38a5b
to
079066f
Compare
This patch implements `RangeKeyChanged()` for `MVCCIncrementalIterator`. It only fires if the time bound range keys change, not if a `Next(Key)IgnoringTime()` operation reveals additional range keys. Release justification: bug fixes and low-risk updates to new functionality Release note: None
This patch changes `MVCCIncrementalIterator` range key behavior following a `Next(Key)IgnoringTime()` call. Previously, range key methods would then expose unfiltered range keys. Now, the standard range key methods only apply to filtered range keys. The additional `RangeKeysIgnoringTime()` and `RangeKeyChangedIgnoringTime()` methods provide access to unfiltered range keys. This implies that if such a call steps onto a range key that's entirely outside of the time bounds then: * `HasPointAndRange()` will return `false`,`false` if on a bare range key. * `RangeKeyChanged()` will not fire, unless stepping off of a range key within the time bounds. * `RangeBounds()` and `RangeKeys()` will return empty results. This is done to avoid conditional range key handling following these calls, except for the exact sites where the caller is interested in the unfiltered range keys. Release justification: bug fixes and low-risk updates to new functionality Release note: None
erikgrinaker
force-pushed
the
mvcc-incremental-iter-rangekeychanged
branch
from
079066f
to
1b064b4
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Done. All standard range key methods now only apply to filtered range keys. Unfiltered range keys are available via
RangeKeysIgnoringTime().The only wart here is that it's now possible for
HasPointAndRange()to returnfalse,falseifNextIgnoringTimesteps onto a bare range key that's outside the time bounds, in violation of theSimpleMVCCIteratorcontract. This isn't really an issue for the current callers, but it is a bit of a footgun so one has to be careful -- then again, one does have to be careful with theIgnoringTimemethods in general.
Meh, had to add in RangeKeyChangedIgnoringTime() too, since it's otherwise possible to land on one of these bare, invisible range keys without detecting it.
See #86512 for the catchup scan "port".
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, 3 of 3 files at r6.
Reviewable status:
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Meh, had to add in
RangeKeyChangedIgnoringTime()too, since it's otherwise possible to land on one of these bare, invisible range keys without detecting it.See #86512 for the catchup scan "port".
Think it's worth removing that last wart, too?
HasPointAndRange() (hasPoint, hasRange, hasRangeIgnoringTime bool)?
It seems valuable to have the contract that for a valid iter, HasPointAndRange() returns at least one true. Most callers could assign while ignoring the third param, if they're pretty sure they didn't call XIgnoringTime.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/mvcc_incremental_iterator.go line 679 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Think it's worth removing that last wart, too?
HasPointAndRange() (hasPoint, hasRange, hasRangeIgnoringTime bool)?It seems valuable to have the contract that for a valid iter,
HasPointAndRange()returns at least onetrue. Most callers could assign while ignoring the third param, if they're pretty sure they didn't callXIgnoringTime.
But that method is part of the SimpleMVCCIterator interface, so we can't add on a return value.
It seems ok to say that if Valid() returns true but HasPointAndRange() returns false,false then we must be on a hidden MVCC range tombstone.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status:
|
TFTR! bors r=tbg |
|
Build succeeded: |
storage: use
RangeKeyChanged()inMVCCIncrementalIteratorThis patch uses
RangeKeyChanged()to detect changes to range keys inMVCCIncrementalIterator. There are no functional changes.Some quick benchmarks, using catchup scans:
Release justification: bug fixes and low-risk updates to new functionality.
Release note: None
storage: implement
RangeKeyChanged()forMVCCIncrementalIteratorThis patch implements
RangeKeyChanged()forMVCCIncrementalIterator.It only fires if the time bound range keys change, not if a
Next(Key)IgnoringTime()operation reveals additional range keys.Resolves #86105.
Release justification: bug fixes and low-risk updates to new functionality
Release note: None
storage: add
MVCCIncrementalIterator.RangeKeysIgnoringTime()This patch changes
MVCCIncrementalIteratorrange key behaviorfollowing a
Next(Key)IgnoringTime()call. Previously, range keymethods would then expose unfiltered range keys. Now, the standard range
key methods only apply to filtered range keys, and an additional
RangeKeysIgnoringTime()method provides access to unfiltered rangekeys.
This implies that if such a call steps onto a range key that's entirely
outside of the time bounds then:
HasPointAndRange()will returnfalse,falseif on a bare rangekey.
RangeKeyChanged()will not fire, unless stepping off of a range keywithin the time bounds.
RangeBounds()andRangeKeys()will return empty results.This is done to avoid conditional range key handling following these
calls, except for the exact sites where the caller is interested in
the unfiltered range keys.
Release justification: bug fixes and low-risk updates to new functionality
Release note: None