diff --git a/db/db_tailing_iter_test.cc b/db/db_tailing_iter_test.cc index 93f12a76d..09ad2cfb6 100644 --- a/db/db_tailing_iter_test.cc +++ b/db/db_tailing_iter_test.cc @@ -358,6 +358,49 @@ TEST_F(DBTestTailingIterator, TailingIteratorSeekToSame) { ASSERT_EQ(found, iter->key().ToString()); } +// Sets iterate_upper_bound and verifies that ForwardIterator doesn't call +// Seek() on immutable iterators when target key is >= prev_key and all +// iterators, including the memtable iterator, are over the upper bound. +TEST_F(DBTestTailingIterator, TailingIteratorUpperBound) { + CreateAndReopenWithCF({"pikachu"}, CurrentOptions()); + + const Slice upper_bound("20", 3); + ReadOptions read_options; + read_options.tailing = true; + read_options.iterate_upper_bound = &upper_bound; + + ASSERT_OK(Put(1, "11", "11")); + ASSERT_OK(Put(1, "12", "12")); + ASSERT_OK(Put(1, "22", "22")); + ASSERT_OK(Flush(1)); // flush all those keys to an immutable SST file + + // Add another key to the memtable. + ASSERT_OK(Put(1, "21", "21")); + + std::unique_ptr it(db_->NewIterator(read_options, handles_[1])); + it->Seek("12"); + ASSERT_TRUE(it->Valid()); + ASSERT_EQ("12", it->key().ToString()); + + it->Next(); + // Not valid since "21" is over the upper bound. + ASSERT_FALSE(it->Valid()); + + // This keeps track of the number of times NeedToSeekImmutable() was true. + int immutable_seeks = 0; + rocksdb::SyncPoint::GetInstance()->SetCallBack( + "ForwardIterator::SeekInternal:Immutable", + [&](void* arg) { ++immutable_seeks; }); + + // Seek to 13. This should not require any immutable seeks. + rocksdb::SyncPoint::GetInstance()->EnableProcessing(); + it->Seek("13"); + rocksdb::SyncPoint::GetInstance()->DisableProcessing(); + + ASSERT_FALSE(it->Valid()); + ASSERT_EQ(0, immutable_seeks); +} + TEST_F(DBTestTailingIterator, ManagedTailingIteratorSingle) { ReadOptions read_options; read_options.tailing = true; diff --git a/db/forward_iterator.cc b/db/forward_iterator.cc index ea493060b..9dbe1bf40 100644 --- a/db/forward_iterator.cc +++ b/db/forward_iterator.cc @@ -128,10 +128,11 @@ ForwardIterator::ForwardIterator(DBImpl* db, const ReadOptions& read_options, sv_(current_sv), mutable_iter_(nullptr), current_(nullptr), + valid_(false), status_(Status::OK()), immutable_status_(Status::OK()), - valid_(false), has_iter_trimmed_for_upper_bound_(false), + current_over_upper_bound_(false), is_prev_set_(false), is_prev_inclusive_(false) { if (sv_) { @@ -179,7 +180,8 @@ void ForwardIterator::Cleanup(bool release_sv) { } bool ForwardIterator::Valid() const { - return valid_; + // See UpdateCurrent(). + return valid_ ? !current_over_upper_bound_ : false; } void ForwardIterator::SeekToFirst() { @@ -225,7 +227,7 @@ void ForwardIterator::SeekInternal(const Slice& internal_key, // an option to turn it off. if (seek_to_first || NeedToSeekImmutable(internal_key)) { immutable_status_ = Status::OK(); - if (NeedToRebuildTrimmed(internal_key)) { + if (has_iter_trimmed_for_upper_bound_) { // Some iterators are trimmed. Need to rebuild. RebuildIterators(true); // Already seeked mutable iter, so seek again @@ -375,6 +377,8 @@ void ForwardIterator::SeekInternal(const Slice& internal_key, is_prev_set_ = true; is_prev_inclusive_ = true; } + + TEST_SYNC_POINT_CALLBACK("ForwardIterator::SeekInternal:Immutable", this); } else if (current_ && current_ != mutable_iter_) { // current_ is one of immutable iterators, push it back to the heap immutable_min_heap_.push(current_); @@ -555,10 +559,13 @@ void ForwardIterator::UpdateCurrent() { if (!status_.ok()) { status_ = Status::OK(); } - if (valid_ && IsOverUpperBound(current_->key())) { - valid_ = false; - current_ = nullptr; - } + + // Upper bound doesn't apply to the memtable iterator. We want Valid() to + // return false when all iterators are over iterate_upper_bound, but can't + // just set valid_ to false, as that would effectively disable the tailing + // optimization (Seek() would be called on all immutable iterators regardless + // of whether the target key is greater than prev_key_). + current_over_upper_bound_ = valid_ && IsOverUpperBound(current_->key()); } bool ForwardIterator::NeedToSeekImmutable(const Slice& target) { @@ -594,26 +601,6 @@ bool ForwardIterator::NeedToSeekImmutable(const Slice& target) { return false; } -bool ForwardIterator::NeedToRebuildTrimmed(const Slice& target) { - if (!has_iter_trimmed_for_upper_bound_) { - return false; - } - if (!valid_ || !current_ || !is_prev_set_ || !immutable_status_.ok()) { - return true; - } - Slice prev_key = prev_key_.GetKey(); - if (prefix_extractor_ && - prefix_extractor_->Transform(target) - .compare(prefix_extractor_->Transform(prev_key)) != 0) { - return true; - } - if (cfd_->internal_comparator().InternalKeyComparator::Compare( - prev_key, target) >= (is_prev_inclusive_ ? 1 : 0)) { - return true; - } - return false; -} - void ForwardIterator::DeleteCurrentIter() { const VersionStorageInfo* vstorage = sv_->current->storage_info(); const std::vector& l0 = vstorage->LevelFiles(0); diff --git a/db/forward_iterator.h b/db/forward_iterator.h index 72a23e757..8ae002d78 100644 --- a/db/forward_iterator.h +++ b/db/forward_iterator.h @@ -79,7 +79,6 @@ class ForwardIterator : public Iterator { void SeekInternal(const Slice& internal_key, bool seek_to_first); void UpdateCurrent(); bool NeedToSeekImmutable(const Slice& internal_key); - bool NeedToRebuildTrimmed(const Slice& internal_key); void DeleteCurrentIter(); uint32_t FindFileInRange( const std::vector& files, const Slice& internal_key, @@ -100,16 +99,30 @@ class ForwardIterator : public Iterator { std::vector l0_iters_; std::vector level_iters_; Iterator* current_; - // internal iterator status + bool valid_; + + // Internal iterator status; set only by one of the unsupported methods. Status status_; + // Status of immutable iterators, maintained here to avoid iterating over + // all of them in status(). Status immutable_status_; - bool valid_; + // Indicates that at least one of the immutable iterators pointed to a key + // larger than iterate_upper_bound and was therefore destroyed. Seek() may + // need to rebuild such iterators. bool has_iter_trimmed_for_upper_bound_; - Slice smallest_file_key_bound; - + // Is current key larger than iterate_upper_bound? If so, makes Valid() + // return false. + bool current_over_upper_bound_; + + // Left endpoint of the range of keys that immutable iterators currently + // cover. When Seek() is called with a key that's within that range, immutable + // iterators don't need to be moved; see NeedToSeekImmutable(). This key is + // included in the range after a Seek(), but excluded when advancing the + // iterator using Next(). IterKey prev_key_; bool is_prev_set_; bool is_prev_inclusive_; + Arena arena_; };