db: fix SetOptions batch range{del,key} refresh logic#1753
db: fix SetOptions batch range{del,key} refresh logic#1753jbowens merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
@nicktrav mind taking a look when you get a chance? This bug is blocking the CockroachDB pebble vendor bump. |
nicktrav
left a comment
There was a problem hiding this comment.
Nice catch.
Reviewed 3 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @itsbilal and @jbowens)
testdata/indexed_batch_mutation line 431 at r2 (raw file):
# Add a new range key to the batch. The batch contains 3 internal range keys, # and the skiplist of fragmented range keys contains 3 elements.
Ah - I see: three fragments, and three range keys, so no refresh. 👍
testdata/indexed_batch_mutation line 443 at r2 (raw file):
iter iter=i1 set-options
Is there any additional assertion worth making on i1 after the batch mutation, prior to set-option? Or are we flogging a dead horse, given all of the existing test cases above?
The commit a015e5a introduced an optimization to avoid refreshing batch rangedel and rangekey iterators when no rangedel or rangekey mutations were applied. This optimization had a bug: It compared unlike numbers to determine whether to refresh. The batch's `count{RangeDels,RangeKeys}` fields record the number of rangedels or range keys added to the batch and are monotonically increasing. The count of spans in the `keyspan.Iter` on the other hand was the count of fragments. This bug could result in erroroneously not refreshing the iterator's view if the previous number of fragments matched the current number of batch keys. This commit adapts SetOptions to not avoid calls to initRangeDelIter and initRangeKeyIter in these cases: These functions already avoid refragmenting the spans if unnecessary, so there should be little cost to calling them unconditionally. The logic to reconstruct the point / range key iterator in SetOptions when a batch goes from zero rangedels/rangekeys to nonzero rangedels/rangekeys is preserved.
jbowens
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @nicktrav)
testdata/indexed_batch_mutation line 443 at r2 (raw file):
Previously, nicktrav (Nick Travers) wrote…
Is there any additional assertion worth making on
i1after the batch mutation, prior toset-option? Or are we flogging a dead horse, given all of the existing test cases above?
added some assertions for completeness
nicktrav
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @itsbilal)
The commit a015e5a introduced an optimization to avoid refreshing batch
rangedel and rangekey iterators when no rangedel or rangekey mutations were
applied.
This optimization had a bug: It compared unlike numbers to determine whether to
refresh. The batch's
count{RangeDels,RangeKeys}fields record the number ofrangedels or range keys added to the batch and are monotonically increasing.
The count of spans in the
keyspan.Iteron the other hand was the count offragments. This bug could result in erroroneously not refreshing the iterator's
view if the previous number of fragments matched the current number of batch
keys.
This commit adapts SetOptions to not avoid calls to initRangeDelIter and
initRangeKeyIter in these cases: These functions already avoid refragmenting
the spans if unnecessary, so there should be little cost to calling them
unconditionally. The logic to reconstruct the point / range key iterator in
SetOptions when a batch goes from zero rangedels/rangekeys to nonzero
rangedels/rangekeys is preserved.