From 4ed79f5bd166245b4e55689de834a372dc4bd68a Mon Sep 17 00:00:00 2001
From: tcwzxx <tcwzxx@gmail.com>
Date: Mon, 2 Dec 2024 13:31:19 -0800
Subject: [PATCH] Fix the issue where compaction incorrectly drops a key when
 there is a snapshot with a sequence number of zero. (#13155)

Summary:
The compaction will incorrectly drop a key under the following conditions:

1. Open an empty database.
2. Use the `IngestExternalFile` API to ingest an SST file (the global sequence number will be 0).
3. Create a snapshot (the snapshot sequence number will be 0).
4. Trigger compaction; the key in the above SST file will be dropped.

The drop condition is found here: https://github.com/facebook/rocksdb/blob/f20d12adc85ece3e75fb238872959c702c0e5535/db/compaction/compaction_iterator.cc#L875-L878
The condition does not explicitly check if a previous key exists.

Fix: Add a check of `last_sequence != kMaxSequenceNumber` to verify if there is a previous key

Pull Request resolved: https://github.com/facebook/rocksdb/pull/13155

Reviewed By: jowlyzhang

Differential Revision: D66473015

Pulled By: cbi42

fbshipit-source-id: 93a3ec5c103f95e9bb97e3944ba6e752a5394421
---
 db/compaction/compaction_iterator.cc          |  4 ++--
 db/compaction/compaction_iterator_test.cc     | 24 +++++++++++++++++++
 .../bug_fixes/compaction_incorrectly_drop.md  |  1 +
 3 files changed, 27 insertions(+), 2 deletions(-)
 create mode 100644 unreleased_history/bug_fixes/compaction_incorrectly_drop.md

diff --git a/db/compaction/compaction_iterator.cc b/db/compaction/compaction_iterator.cc
index 1b69a9ecfd..a8226ecbb4 100644
--- a/db/compaction/compaction_iterator.cc
+++ b/db/compaction/compaction_iterator.cc
@@ -872,8 +872,8 @@ void CompactionIterator::NextFromInput() {
       if (Valid()) {
         at_next_ = true;
       }
-    } else if (last_snapshot == current_user_key_snapshot_ ||
-               (last_snapshot > 0 &&
+    } else if (last_sequence != kMaxSequenceNumber &&
+               (last_snapshot == current_user_key_snapshot_ ||
                 last_snapshot < current_user_key_snapshot_)) {
       // If the earliest snapshot is which this key is visible in
       // is the same as the visibility of a previous instance of the
diff --git a/db/compaction/compaction_iterator_test.cc b/db/compaction/compaction_iterator_test.cc
index e3b4575f56..c3a8a7574b 100644
--- a/db/compaction/compaction_iterator_test.cc
+++ b/db/compaction/compaction_iterator_test.cc
@@ -833,6 +833,14 @@ TEST_P(CompactionIteratorTest, ConvertToPutAtBottom) {
           true /*bottomost_level*/);
 }
 
+TEST_P(CompactionIteratorTest, ZeroSeqOfKeyAndSnapshot) {
+  AddSnapshot(0);
+  const std::vector<std::string> input_keys = {
+      test::KeyStr("a", 0, kTypeValue), test::KeyStr("b", 0, kTypeValue)};
+  const std::vector<std::string> input_values = {"a1", "b1"};
+  RunTest(input_keys, input_values, input_keys, input_values);
+}
+
 INSTANTIATE_TEST_CASE_P(CompactionIteratorTestInstance, CompactionIteratorTest,
                         testing::Values(true, false));
 
@@ -1846,6 +1854,22 @@ TEST_P(CompactionIteratorTsGcTest, SingleDeleteAllKeysOlderThanThreshold) {
   }
 }
 
+TEST_P(CompactionIteratorTsGcTest, ZeroSeqOfKeyAndSnapshot) {
+  AddSnapshot(0);
+  std::string full_history_ts_low;
+  PutFixed64(&full_history_ts_low, std::numeric_limits<uint64_t>::max());
+  const std::vector<std::string> input_keys = {
+      test::KeyStr(101, "a", 0, kTypeValue),
+      test::KeyStr(102, "b", 0, kTypeValue)};
+  const std::vector<std::string> input_values = {"a1", "b1"};
+  RunTest(input_keys, input_values, input_keys, input_values,
+          /*last_committed_seq=*/kMaxSequenceNumber,
+          /*merge_operator=*/nullptr, /*compaction_filter=*/nullptr,
+          /*bottommost_level=*/false,
+          /*earliest_write_conflict_snapshot=*/kMaxSequenceNumber,
+          /*key_not_exists_beyond_output_level=*/false, &full_history_ts_low);
+}
+
 INSTANTIATE_TEST_CASE_P(CompactionIteratorTsGcTestInstance,
                         CompactionIteratorTsGcTest,
                         testing::Values(true, false));
diff --git a/unreleased_history/bug_fixes/compaction_incorrectly_drop.md b/unreleased_history/bug_fixes/compaction_incorrectly_drop.md
new file mode 100644
index 0000000000..df35033452
--- /dev/null
+++ b/unreleased_history/bug_fixes/compaction_incorrectly_drop.md
@@ -0,0 +1 @@
+Fix the issue where compaction incorrectly drops a key when there is a snapshot with a sequence number of zero.