batcheval: handle MVCC range tombstones in RefreshRange
#83386
Conversation
|
This change is |
|
|
||
| if _, hasRange := iter.HasPointAndRange(); hasRange { | ||
| rangeKVs := iter.RangeKeys() | ||
| if len(rangeKVs) == 0 { // defensive |
There was a problem hiding this comment.
Do we need to be defensive here because of the time bounds that could filter all range keys while still reporting has range?
There was a problem hiding this comment.
Sort of. The MVCCIterator contract specifies that if hasRange is true, then RangeKeys() must be non-empty. I don't have any reason to believe our iterators don't satisfy that contract (MVCCIncrementalIterator respects range key filtering in HasPointAndRange), but if an iterator doesn't then we'll hit an out-of-bounds panic on the [0] access below and crash the node. Seemed better to be defensive and error instead of panic'ing.
| Key: start, | ||
| Timestamp: ts2, | ||
| }, *refreshErr) | ||
| } |
There was a problem hiding this comment.
Do we also need a test where all ranges are below the RefreshFrom to see that it passes as expected without any errors?
There was a problem hiding this comment.
Yeah, we should. The existing tests didn't, and I was just being lazy. Will add some cases both for point and range keys.
This patch add handling of MVCC range tombstones in `RefreshRange`, by erroring on them as a committed value. Release note: None
erikgrinaker
force-pushed
the
mvcc-range-tombstones-refresh-range
branch
from
49b01ab
to
ecfed3b
Compare
|
TFTR! bors r=aliher1911 |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
83386: batcheval: handle MVCC range tombstones in `RefreshRange` r=aliher1911 a=erikgrinaker This patch add handling of MVCC range tombstones in `RefreshRange`, by erroring on them as a committed value. Resolves #83383. Release note: None 83562: rowexec: fix recent bug of using nil context r=yuzefovich a=yuzefovich In e7e724e we moved the creation of a monitor for the streamer's disk usage into the constructor of the join reader but forgot to update the context used during that operation. The thing is that the context on the processors is only set in `Start` meaning it is `nil` in the construct of the processor. This commit fixes the issue. Fixes: #83367 Release note: None Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This patch add handling of MVCC range tombstones in
RefreshRange, byerroring on them as a committed value.
Resolves #83383.
Release note: None