[WritePreparedTxn] CompactionIterator sees consistent view of which keys are committed

[riversand963]

This PR does not affect the functionality of DB and write-committed transactions.

CompactionIterator uses KeyCommitted(seq) to determine if a key in the database is committed.
As the name 'write-committed' implies, if write-committed policy is used, a key exists in the database only if
it is committed. In fact, the implementation of KeyCommitted() is as follows:

inline bool KeyCommitted(SequenceNumber seq) {
  // For non-txn-db and write-committed, snapshot_checker_ is always nullptr.
  return snapshot_checker_ == nullptr ||
         snapshot_checker_->CheckInSnapshot(seq, kMaxSequence) == SnapshotCheckerResult::kInSnapshot;
}

With that being said, we focus on write-prepared/write-unprepared transactions.

A few notes:

• A key can exist in the db even if it's uncommitted. Therefore, we rely on snapshot_checker_ to determine data visibility. We also require that all writes go through transaction API instead of the raw WriteBatch + Write, thus at most one uncommitted version of one user key can exist in the database.
• CompactionIterator outputs a key as long as the key is uncommitted.

Due to the above reasons, it is possible that CompactionIterator decides to output an uncommitted key without
doing further checks on the key (NextFromInput()). By the time the key is being prepared for output, the key becomes
committed because the snapshot_checker_(seq, kMaxSequence) becomes true in the implementation of KeyCommitted().
Then CompactionIterator will try to zero its sequence number and hit assertion error if the key is a tombstone.

To fix this issue, we should make the CompactionIterator see a consistent view of the input keys. Note that
for write-prepared/write-unprepared, the background flush/compaction jobs already take a "job snapshot" before starting
processing keys. The job snapshot is released only after the entire flush/compaction finishes. We can use this snapshot
to determine whether a key is committed or not with minor change to KeyCommitted().

inline bool KeyCommitted(SequenceNumber sequence) {
  // For non-txn-db and write-committed, snapshot_checker_ is always nullptr.
  return snapshot_checker_ == nullptr ||
         snapshot_checker_->CheckInSnapshot(sequence, job_snapshot_) ==
             SnapshotCheckerResult::kInSnapshot;
}

As a result, whether a key is committed or not will remain a constant throughout compaction, causing no trouble
for CompactionIterators assertions.

Test plan:
make check

[facebook-github-bot]

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@riversand963 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ltamasi]

we should make the CompactionIterator see a consistent view of the input keys

I recall discussing this a while back; didn't realize it was this easy :)

[riversand963]

we should make the CompactionIterator see a consistent view of the input keys

I recall discussing this a while back; didn't realize it was this easy :)

I should have been more specific about the PR as "CompactionIterator sees consistent view of what's committed".
There is also the issue of CompactionIterator seeing consistent view of what keys are visible in which snapshot. This is not easy in the current implementation of write-prepared since snapshots can be dropped, thus returning different results for IsDefinitelyInSnapshot() and IsDefinitelyNotInSnapshot().

[ltamasi]

Thanks for the PR @riversand963 !

[facebook-github-bot]

@riversand963 has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[riversand963]

Thanks @ltamasi for the review!