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 8a5c4524055016e2ecc64253d6d5018dd3e3401a..98581b16d7626cb137b8636a528803ee7002c4f7 100644 (file)
--- 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 0fa3fcd3ebe4cb69e824cc76da9e4bdeb9928aca..ae92aa19830ef021c72332b829d0d6071939c6b2 100644 (file)
--- 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 (file)
index 0000000..1cedc72
--- /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