From: Changyu Bi <changyubi@meta.com>
Date: Fri, 1 Sep 2023 16:34:08 +0000 (-0700)
Subject: Fix a bug where iterator status is not checked (#11782)
X-Git-Tag: v8.4.4~2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=e283b751a69e65f6d041029c4241548b667f22a8;p=rocksdb.git

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
---

diff --git a/table/compaction_merging_iterator.cc b/table/compaction_merging_iterator.cc
index 8a5c45240..98581b16d 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 0fa3fcd3e..ae92aa198 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 000000000..1cedc7215
--- /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