Verify explicitly synced data is recoverable

[ajkr]

This issue is a next step mentioned in my blog post (https://rocksdb.org/blog/2022/10/05/lost-buffered-write-recovery.html):

An untested guarantee is that RocksDB recovers all writes that the user explicitly flushed out of the buffers lost in the crash. We may recover more writes than these due to internal flushing of buffers, but never less. Our test oracle needs to be further extended to track the lower bound on the sequence number that is expected to survive a crash.

The first step is to handle the single column family, WAL disabled case. We could use either seqno for tracking or a new write counter internal to the test oracle. We should update the expected minimum value of that counter for (1) explicit flushes, and (2) OnFlushCompleted() events. We should verify the recovery reaches at least the expected minimum value.

[hx235]

In case it's not already obvious, for WAL disabled case, we will only consider with atomic_flush=true. For a flush, we do not need to verify recoverability of writes concurrent to that flush as there is no guarantee from RocksDB on whether that concurrent writes are included in that flush or not.

[jacobxli-xx]

hi, ajkr@ and hx235@. Glad to take this task : )

[ajkr]

@jacobxli-xx Thank you, it is assigned now.

[ajkr]

@jacobxli-xx Here are some questions I had about what you are planning to do:

1) For explicit sync mechanisms, will the scope be limited to verifying writes that completed before the explicit sync are recovered? 2) For implicit sync mechanisms (i.e., automatic flush), will the scope be limited to verifying writes that are persisted according to OnFlushCompleted()?

If any answers are no, we'll probably want to know the reason and eventually the plan to make a guess of whether we'd be on board with it.

[jacobxli-xx]

Hey, Andrew. The answer for the both questions are yes. For both explicit flush and auto flush, we are going to limit the scope of verification to recover all writes till the maximum sequence number of write recorded before onFlushCompleted() (test cases limited to only 1 column family).

[hx235]

@jacobxli-xx If you are considering storing and m-mapping map<std::string, uint64_t> cf_flushed_seqno, I wonder why can't we just store and mmap the maximum of all the seqnos in the map?

[jacobxli-xx]

@hx235 The m-mapping map<std::string, uint64_t> cf_flushed_seqno stores the cf name and the corresponding maximum flushed seqno of the specific cf. Why we do not only store the maximum of all the seqnos is because when we do explicit flush on specific cf, it's not guaranteed that all cfs will be flushed, so the maximum of all seqno is not guaranteed to be equal to the maximum seqno of the cf we flushed.

Although our test cases are limited to 1 cf, we use this map for forward compatibility as it can be used in multiple cfs cases.

[ajkr]

because when we do explicit flush on specific cf, it's not guaranteed that all cfs will be flushed, so the maximum of all seqno is not guaranteed to be equal to the maximum seqno of the cf we flushed.

It is guaranteed in the case where it is needed (atomic_flush=true) - https://github.com/facebook/rocksdb/blob/3ef909248736eb80e7c6b3c7a19753c4dbe9d0c5/db_stress_tool/db_stress_test_base.cc#L2266-L2268

It is not guaranteed when WAL is enabled and atomic_flush is disabled, but there it is not needed because WAL ensures consistent recovery across column families.

WAL-disabled, atomic_flush-disabled may be worth testing at some future point but that is still an open topic to discuss. My thoughts are here - #11841.

Another case that may be worth testing at some point is WAL-disabled, atomic_flush-enabled, and manual flush triggered on a subset of column families (what you suggested we are already doing). Still, it's not something we decided to do yet, so adding extra complexity assuming we will do it feels premature

[jacobxli-xx]

The thoughts on WAL-disabled, atomic_flush-disabled cases in #11841 sounds reasonable to me. Using only one seqno should be fine. Will modify the implementation a bit accordingly.

[jacobxli-xx]

Hi, an update on the status. Planning on sending out the PR by Apr 10th.