libroach: prevent infinite loop in reverse-scan

[ajkr]

Fixed and added a test case for infinite loop leading to OOM in reverse-scan
optimization to use SeekForPrev() after N Prev()s fail to find a new
key. It could happen when that optimization was used on a key that also
had a write intent.

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr)

---

c-deps/libroach/mvcc.h, line 552 at r1 (raw file):

    // Since the iterator we're using here has its upper-bound set to `key`,
    // this seeks to the last key that is strictly less than `key`.
    assert(key.compare(iter_->upper_bound) == 0);

I'm not sure if this is safe. We also call iterSeekReverse from prevKey.

[ajkr]

I see. Does that usage also rely on SeekForPrev landing at a key strictly less than the seek key? It appears that way to me because the seek key is the current key with zero timestamp, which might be an actual key.

[petermattis]

I see. Does that usage also rely on SeekForPrev landing at a key strictly less than the seek key? It appears that way to me because the seek key is the current key with zero timestamp, which might be an actual key.

Yes, it does rely on SeekForPrev landing at a key strictly less than the seek key. Is there a bug here? When will SeekForPrev return a key equal to the seek key? I'm pretty sure I both tested this and looked at the source code when I wrote that comment.

[ajkr]

I think it should land on the seek key if it exists in the DB and isn't excluded by iterate_upper_bound, e.g., https://github.com/facebook/rocksdb/blob/master/db/db_iter_test.cc#L2264-L2266

[petermattis]

I think it should land on the seek key if it exists in the DB and isn't excluded by iterate_upper_bound, e.g., https://github.com/facebook/rocksdb/blob/master/db/db_iter_test.cc#L2264-L2266

I'll look at the code in RocksDB later. Seems feasible to test this scenario and demonstrate a problem.

[ajkr]

Got it. Will attempt to expose a problem in a test case in mvcc_test.go.

[ajkr]

Fun investigation. I added a test case and reverted the change to iterSeekReverse.

EDIT: But I did not solve the actual problem yet.

[petermattis]

Well, definitely a bug. I wonder if we've ever hit this in practice. It requires reverse scans, and a number of versions for a key, which might be a rare combination. Glad you found this, though.

[ajkr]

This is ready for review.

[petermattis]

Very glad you caught this.

Can you run the MVCCReverseScan benchmarks? I'm curious to see how this affected them, though I don't see an easy way to avoid this check.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajkr)

[ajkr]

Sure, the benchmark looks the same more or less (https://gist.github.com/ajkr/c7059d4dd38e111d91908d7d1eb244b0), so I think this should be safe to land.

[ajkr]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)