c-deps: Investigate RocksDB DeleteRange bug

[bdarnell]

The TiDB team found a data corruption bug in RocksDB (blog post, issue, PR). We need to determine whether we are affected by this; if so we need to patch in the fix for 1.1.

The bug relates to the DeleteRange operation and snapshots; we use both features and based on the test case in facebook/rocksdb#2799 I don't see any reason to assume that we're immune. The symptom is data that was deleted by DeleteRange could reappear.

[petermattis]

Sounds like the "snapshots" being referred to are TiKV range-level snapshots and the bug requires a combination of DeleteRange + IngestExternalFile. If that is the case, it would only affect restore. Still reading about this, I could be mistaken.

Cc @danhhz, @benesch

[petermattis]

Ah, the TiKV-folks talk about using IngestExternalFile, but the test case only uses RocksDB snapshots.

[petermattis]

I ported the test case in https://github.com/facebook/rocksdb/pull/2799/files to our code base yet I'm not seeing a failure. Sort of curious that we haven't seen any consistency check failures recently. We've been using DeleteRange for a while and this isn't a new bug.

[petermattis]

Oh, looks like their test is missing a CompactRange. After adding that, I can reproduce. I'm not seeing any reason this couldn't happen in our code. It just requires a specific order of DeleteRange, snapshots and compactions.

One mitigating factor is that it looks like an explicit snapshot created via DB::GetSnapshot is necessary. The implicit snapshot used by iterators doesn't seem to tickle the bug. That said, we still use snapshots frequently, such as during rebalancing and splitting.

I'm going to attempt to reproduce by convincing replica GC and rebalancing to create the correct intermingling of RocksDB operations. It is possible that one of the existing remove-and-re-add tests sees this bug, but a) we're not checking for it and b) we don't use DeleteRange unless there are a certain number of keys.

[petermattis]

Btw, the fix is relatively small and I've verified it fixes my reproduction. We might want to just apply it.

[benesch]

Agreed, given the size of the patch I think we should just apply it. What are the other options for 1.1? Disabling DeleteRange entirely?

[bdarnell]

Yeah, let's just apply it.

[petermattis]

Ok. I'll add it to #18374.

[bdarnell]

Since the bug's been around a while, should we backport it to 1.0 too?

[petermattis]

Since the bug's been around a while, should we backport it to 1.0 too?

Are we going to do a 1.0.7? Or should we squeeze it into 1.0.6? I can whip up a cherrypick quickly.

[bdarnell]

I think if it's ready today we could put it in 1.0.6 (we haven't started qualifying a specific build yet)

[petermattis]

The RocksDB patch didn't apply cleanly to the version of RocksDB we're using on the release-1.0 branch.