crl-release-20.2: db: mergingIter.SeekPrefixGE stops early if prefix cannot match

[jbowens]

This is a 20.2 backport of #1085. While investigating cockroachlabs/support#898,
one node hit #1070:

The cherrypick didn't apply cleanly because of other recent iterator optimizations,
so I think we should probably wait for @sumeerbhola to give this a review before
merging.

name                                                            old time/op  new time/op  delta
IteratorSeqSeekPrefixGENotFound/skip=1/with-tombstone=false-16  1.42µs ± 4%  1.36µs ± 6%     ~     (p=0.095 n=5+5)
IteratorSeqSeekPrefixGENotFound/skip=1/with-tombstone=true-16   11.0ms ± 5%   0.0ms ± 6%  -99.99%  (p=0.008 n=5+5)
IteratorSeqSeekPrefixGENotFound/skip=2/with-tombstone=false-16  1.39µs ± 4%  1.36µs ±11%     ~     (p=0.310 n=5+5)
IteratorSeqSeekPrefixGENotFound/skip=2/with-tombstone=true-16   11.2ms ± 5%   0.0ms ± 4%  -99.99%  (p=0.008 n=5+5)
IteratorSeqSeekPrefixGENotFound/skip=4/with-tombstone=false-16  1.38µs ± 4%  1.38µs ± 7%     ~     (p=1.000 n=5+5)
IteratorSeqSeekPrefixGENotFound/skip=4/with-tombstone=true-16   11.1ms ± 3%   0.0ms ± 9%  -99.99%  (p=0.008 n=5+5)

[cockroach-teamcity]

This change is 

[petermattis]

Which part of the backport didn't apply cleanly? The change to merging_iter.go looks pretty straightforward.

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @andreimatei, @itsbilal, @nvanbenschoten, and @sumeerbhola)

[jbowens]

Which part of the backport didn't apply cleanly? The change to merging_iter.go looks pretty straightforward.

Mostly test code and datadriven test expectations. Specifically, the seek-prefix-ge c result changed.

I think the change in expectations makes sense, because we don't have some of the iterator changes Sumeer made and both outputs are valid within the internalIterator's contract. @sumeerbhola's commentary within the datadriven test explains why on master the test is allowed to return d for seek-prefix-ge c, but it's equally valid to return not found.

# prefix, and when it steps to d, it finds d is not deleted. Note that
# mergingIter is an InternalIterator and does not need to guarantee prefix
# match -- that is job of the higher-level Iterator. So "seek-prefix-ge c" is
# allowed to return d.

If you're comfortable with this since the actual non-test code changes are straightforward and minimal, we could merge it today.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @andreimatei, @itsbilal, @nvanbenschoten, and @sumeerbhola)

---

testdata/merging_iter, line 529 at r1 (raw file):

Quoted 4 lines of code…

# prefix, and when it steps to d, it finds d is not deleted. Note that
# mergingIter is an InternalIterator and does not need to guarantee prefix
# match -- that is job of the higher-level Iterator. So "seek-prefix-ge c" is
# allowed to return d.

[itsbilal]

Reviewed 2 of 6 files at r1.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @andreimatei, @nvanbenschoten, and @sumeerbhola)

[sumeerbhola]

thanks for backporting this!

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andreimatei, @jbowens, and @nvanbenschoten)

---

testdata/merging_iter, line 529 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

# prefix, and when it steps to d, it finds d is not deleted. Note that
# mergingIter is an InternalIterator and does not need to guarantee prefix
# match -- that is job of the higher-level Iterator. So "seek-prefix-ge c" is
# allowed to return d.

Can you update the comment here since seek-prefix-ge c is not finding d.

[jbowens]

TFTRs!

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @andreimatei, @nvanbenschoten, and @sumeerbhola)

---

testdata/merging_iter, line 529 at r1 (raw file):

Previously, sumeerbhola wrote…

Can you update the comment here since seek-prefix-ge c is not finding d.

Done