From bd6a8340c3a2db764620e90b3ac5be173fc68a0c Mon Sep 17 00:00:00 2001
From: Changyu Bi <changyubi@meta.com>
Date: Fri, 1 Sep 2023 09:34:08 -0700
Subject: [PATCH] Fix a bug where iterator status is not checked (#11782)

Summary:
This happens in (Compaction)MergingIterator layer, and can cause data loss during compaction or read/scan return incorrect result

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

Reviewed By: ajkr

Differential Revision: D48880575

Pulled By: cbi42

fbshipit-source-id: 2294ad284a6d653d3674bebe55380f12ee4b645b
---
 table/compaction_merging_iterator.cc                      | 1 +
 table/merging_iterator.cc                                 | 8 ++++++++
 .../bug_fixes/001_check_iter_status_data_loss.md          | 1 +
 3 files changed, 10 insertions(+)
 create mode 100644 unreleased_history/bug_fixes/001_check_iter_status_data_loss.md

diff --git a/table/compaction_merging_iterator.cc b/table/compaction_merging_iterator.cc
index 8a5c4524055..98581b16d76 100644
--- a/table/compaction_merging_iterator.cc
+++ b/table/compaction_merging_iterator.cc
@@ -329,6 +329,7 @@ void CompactionMergingIterator::FindNextVisibleKey() {
       assert(current->iter.status().ok());
       minHeap_.replace_top(current);
     } else {
+      considerStatus(current->iter.status());
       minHeap_.pop();
     }
     if (range_tombstone_iters_[current->level]) {
diff --git a/table/merging_iterator.cc b/table/merging_iterator.cc
index 0fa3fcd3ebe..ae92aa19830 100644
--- a/table/merging_iterator.cc
+++ b/table/merging_iterator.cc
@@ -931,6 +931,7 @@ bool MergingIterator::SkipNextDeleted() {
       InsertRangeTombstoneToMinHeap(current->level, true /* start_key */,
                                     true /* replace_top */);
     } else {
+      // TruncatedRangeDelIterator does not have status
       minHeap_.pop();
     }
     return true /* current key deleted */;
@@ -988,6 +989,9 @@ bool MergingIterator::SkipNextDeleted() {
     if (current->iter.Valid()) {
       assert(current->iter.status().ok());
       minHeap_.push(current);
+    } else {
+      // TODO(cbi): check status and early return if non-ok.
+      considerStatus(current->iter.status());
     }
     // Invariants (rti) and (phi)
     if (range_tombstone_iters_[current->level] &&
@@ -1027,6 +1031,7 @@ bool MergingIterator::SkipNextDeleted() {
         if (current->iter.Valid()) {
           minHeap_.replace_top(current);
         } else {
+          considerStatus(current->iter.status());
           minHeap_.pop();
         }
         return true /* current key deleted */;
@@ -1199,6 +1204,8 @@ bool MergingIterator::SkipPrevDeleted() {
     if (current->iter.Valid()) {
       assert(current->iter.status().ok());
       maxHeap_->push(current);
+    } else {
+      considerStatus(current->iter.status());
     }
 
     if (range_tombstone_iters_[current->level] &&
@@ -1241,6 +1248,7 @@ bool MergingIterator::SkipPrevDeleted() {
         if (current->iter.Valid()) {
           maxHeap_->replace_top(current);
         } else {
+          considerStatus(current->iter.status());
           maxHeap_->pop();
         }
         return true /* current key deleted */;
diff --git a/unreleased_history/bug_fixes/001_check_iter_status_data_loss.md b/unreleased_history/bug_fixes/001_check_iter_status_data_loss.md
new file mode 100644
index 00000000000..1cedc721510
--- /dev/null
+++ b/unreleased_history/bug_fixes/001_check_iter_status_data_loss.md
@@ -0,0 +1 @@
+* Fix a bug where if there is an error reading from offset 0 of a file from L1+ and that the file is not the first file in the sorted run, data can be lost in compaction and read/scan can return incorrect results.
\ No newline at end of file