Skip to content

Commit

Permalink
Fix a bug where iterator can return incorrect data for DeleteRange() …
Browse files Browse the repository at this point in the history
…users (#11786)

Summary:
This should only affect iterator when
- user uses DeleteRange(),
- An iterator from level L has a non-ok status (such non-ok status may not be caught before the bug fix in #11783), and
- A range tombstone covers a key from level > L and triggers a reseek sets the status_ to OK in SeekImpl()/SeekPrevImpl() e.g. https://github.com/facebook/rocksdb/blob/bd6a8340c3a2db764620e90b3ac5be173fc68a0c/table/merging_iterator.cc#L801

Pull Request resolved: #11786

Differential Revision: D48908830

Pulled By: cbi42

fbshipit-source-id: eb564be375af4e33dc27542eff753260186e6d5d
  • Loading branch information
cbi42 authored and ajkr committed Sep 1, 2023
1 parent e283b75 commit f2cbed0
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
4 changes: 2 additions & 2 deletions table/merging_iterator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ class MergingIterator : public InternalIterator {
// holds after this call, and minHeap_.top().iter points to the
// first key >= target among children_ that is not covered by any range
// tombstone.
status_ = Status::OK();
SeekImpl(target);
FindNextVisibleKey();

Expand All @@ -321,6 +322,7 @@ class MergingIterator : public InternalIterator {
void SeekForPrev(const Slice& target) override {
assert(range_tombstone_iters_.empty() ||
range_tombstone_iters_.size() == children_.size());
status_ = Status::OK();
SeekForPrevImpl(target);
FindPrevVisibleKey();

Expand Down Expand Up @@ -798,7 +800,6 @@ void MergingIterator::SeekImpl(const Slice& target, size_t starting_level,
active_.erase(active_.lower_bound(starting_level), active_.end());
}

status_ = Status::OK();
IterKey current_search_key;
current_search_key.SetInternalKey(target, false /* copy */);
// Seek target might change to some range tombstone end key, so
Expand Down Expand Up @@ -1083,7 +1084,6 @@ void MergingIterator::SeekForPrevImpl(const Slice& target,
active_.erase(active_.lower_bound(starting_level), active_.end());
}

status_ = Status::OK();
IterKey current_search_key;
current_search_key.SetInternalKey(target, false /* copy */);
// Seek target might change to some range tombstone end key, so
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Fix a bug where iterator may return incorrect result for DeleteRange() users if there was an error reading from a file.

0 comments on commit f2cbed0

Please sign in to comment.