Fix a bug in MultiScan that moves iterator backward

[cbi42]

Summary: MultiScanUnexpectedSeekTarget() currently uses user key comparison to decide on the next data block for multiscan. This can cause a multiscan to move backward in the following scenario:

data block 1: ..., k@7, k@6
data block 2: k@5, ...

DB iter scan through k@7, k@6 and k@5 and decides to seek to k@0 due to option max_sequential_skip_in_iterations. Multiscan was on data block 2, but moves to data block 1 after the seek.

This can cause assertion failure in debug mode and seg fault in prod since older data blocks are unpinned and freed as we advanced a multiscan. This PR fixes the issue by forcing a multiscan to never go backward.

Test plan:

• added a new unit test that reproduces the scenario: ./db_iterator_test --gtest_filter="*ReseekAcrossBlocksSameUserKey*"

[meta-codesync]

@cbi42 has imported this pull request. If you are a Meta employee, you can view this in D86428845.

[xingbowang]

Thanks for fixing this. Looks good to me.

I have 2 follow up questions.

1. What if the same key span 2 files with multiple verions?
2. Would engine run into some kind of dead loop? if it repeatedly do re-seek? Or there is some kind of upper level logic to ensure it only reseek once?

[cbi42]

1. What if the same key span 2 files with multiple verions?

I don't think it makes a difference. LevelIterator::Seek() will pick the right file based on the seek key and file key ranges.

2. Would engine run into some kind of dead loop? if it repeatedly do re-seek? Or there is some kind of upper level logic to ensure it only reseek once?

It looks like we only reseek once for a user key:

rocksdb/db/db_iter.cc

Lines 564 to 567 in 37176a4

	if (num_skipped > max_skip_ && !reseek_done) {
	is_key_seqnum_zero_ = false;
	num_skipped = 0;
	reseek_done = true;

If we seek to the last internal key for a user key, I don't think it will trigger another reseek.

[meta-codesync]

@cbi42 merged this pull request in ea75cdc.