diff --git a/HISTORY.md b/HISTORY.md index 2a3e69830..762162670 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,7 @@ # Rocksdb Change Log ## Unreleased +### Bug Fixes +* Revert the feature "Merging iterator to avoid child iterator reseek for some cases (#5286)" since it might cause strong results when reseek happens with a different iterator upper bound. ## 6.5.0 (9/13/2019) ### Bug Fixes @@ -104,7 +106,6 @@ * Fix a bug caused by secondary not skipping the beginning of new MANIFEST. * On DB open, delete WAL trash files left behind in wal_dir - ## 6.2.0 (4/30/2019) ### New Features * Add an option `strict_bytes_per_sync` that causes a file-writing thread to block rather than exceed the limit on bytes pending writeback specified by `bytes_per_sync` or `wal_bytes_per_sync`. diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index 997b38602..77ca78010 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -2690,75 +2690,6 @@ TEST_P(DBIteratorTest, AvoidReseekLevelIterator) { SyncPoint::GetInstance()->DisableProcessing(); } -TEST_P(DBIteratorTest, AvoidReseekChildIterator) { - Options options = CurrentOptions(); - options.compression = CompressionType::kNoCompression; - BlockBasedTableOptions table_options; - table_options.block_size = 800; - options.table_factory.reset(NewBlockBasedTableFactory(table_options)); - Reopen(options); - - Random rnd(301); - std::string random_str = RandomString(&rnd, 180); - - ASSERT_OK(Put("1", random_str)); - ASSERT_OK(Put("2", random_str)); - ASSERT_OK(Put("3", random_str)); - ASSERT_OK(Put("4", random_str)); - ASSERT_OK(Put("8", random_str)); - ASSERT_OK(Put("9", random_str)); - ASSERT_OK(Flush()); - ASSERT_OK(Put("5", random_str)); - ASSERT_OK(Put("6", random_str)); - ASSERT_OK(Put("7", random_str)); - ASSERT_OK(Flush()); - - // These two keys will be kept in memtable. - ASSERT_OK(Put("0", random_str)); - ASSERT_OK(Put("8", random_str)); - - int num_iter_wrapper_seek = 0; - SyncPoint::GetInstance()->SetCallBack( - "IteratorWrapper::Seek:0", - [&](void* /*arg*/) { num_iter_wrapper_seek++; }); - SyncPoint::GetInstance()->EnableProcessing(); - { - std::unique_ptr iter(NewIterator(ReadOptions())); - iter->Seek("1"); - ASSERT_TRUE(iter->Valid()); - // DBIter always wraps internal iterator with IteratorWrapper, - // and in merging iterator each child iterator will be wrapped - // with IteratorWrapper. - ASSERT_EQ(4, num_iter_wrapper_seek); - - // child position: 1 and 5 - num_iter_wrapper_seek = 0; - iter->Seek("2"); - ASSERT_TRUE(iter->Valid()); - ASSERT_EQ(3, num_iter_wrapper_seek); - - // child position: 2 and 5 - num_iter_wrapper_seek = 0; - iter->Seek("6"); - ASSERT_TRUE(iter->Valid()); - ASSERT_EQ(4, num_iter_wrapper_seek); - - // child position: 8 and 6 - num_iter_wrapper_seek = 0; - iter->Seek("7"); - ASSERT_TRUE(iter->Valid()); - ASSERT_EQ(3, num_iter_wrapper_seek); - - // child position: 8 and 7 - num_iter_wrapper_seek = 0; - iter->Seek("5"); - ASSERT_TRUE(iter->Valid()); - ASSERT_EQ(4, num_iter_wrapper_seek); - } - - SyncPoint::GetInstance()->DisableProcessing(); -} - // MyRocks may change iterate bounds before seek. Simply test to make sure such // usage doesn't break iterator. TEST_P(DBIteratorTest, IterateBoundChangedBeforeSeek) { diff --git a/db/version_set.cc b/db/version_set.cc index e3c2397cd..36e9c527d 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -859,8 +859,7 @@ class LevelIterator final : public InternalIterator { bool skip_filters, int level, RangeDelAggregator* range_del_agg, const std::vector* compaction_boundaries = nullptr) - : InternalIterator(false), - table_cache_(table_cache), + : table_cache_(table_cache), read_options_(read_options), env_options_(env_options), icomparator_(icomparator), diff --git a/table/block_based/block_based_table_reader.h b/table/block_based/block_based_table_reader.h index c8e8ea006..f9345c42b 100644 --- a/table/block_based/block_based_table_reader.h +++ b/table/block_based/block_based_table_reader.h @@ -615,8 +615,7 @@ class BlockBasedTableIterator : public InternalIteratorBase { const SliceTransform* prefix_extractor, BlockType block_type, TableReaderCaller caller, size_t compaction_readahead_size = 0) - : InternalIteratorBase(false), - table_(table), + : table_(table), read_options_(read_options), icomp_(icomp), user_comparator_(icomp.user_comparator()), diff --git a/table/internal_iterator.h b/table/internal_iterator.h index adcccf795..d7940eeff 100644 --- a/table/internal_iterator.h +++ b/table/internal_iterator.h @@ -25,8 +25,8 @@ struct IterateResult { template class InternalIteratorBase : public Cleanable { public: - InternalIteratorBase() : is_mutable_(true) {} - InternalIteratorBase(bool _is_mutable) : is_mutable_(_is_mutable) {} + InternalIteratorBase() {} + // No copying allowed InternalIteratorBase(const InternalIteratorBase&) = delete; InternalIteratorBase& operator=(const InternalIteratorBase&) = delete; @@ -148,7 +148,6 @@ class InternalIteratorBase : public Cleanable { virtual Status GetProperty(std::string /*prop_name*/, std::string* /*prop*/) { return Status::NotSupported(""); } - bool is_mutable() const { return is_mutable_; } protected: void SeekForPrevImpl(const Slice& target, const Comparator* cmp) { diff --git a/table/iterator_wrapper.h b/table/iterator_wrapper.h index a5aa5c49e..33c52e1b2 100644 --- a/table/iterator_wrapper.h +++ b/table/iterator_wrapper.h @@ -73,7 +73,6 @@ class IteratorWrapperBase { } void Prev() { assert(iter_); iter_->Prev(); Update(); } void Seek(const Slice& k) { - TEST_SYNC_POINT("IteratorWrapper::Seek:0"); assert(iter_); iter_->Seek(k); Update(); diff --git a/table/merging_iterator.cc b/table/merging_iterator.cc index 1a0d4df89..a76b91e75 100644 --- a/table/merging_iterator.cc +++ b/table/merging_iterator.cc @@ -127,29 +127,12 @@ class MergingIterator : public InternalIterator { } void Seek(const Slice& target) override { - bool is_increasing_reseek = false; - if (current_ != nullptr && direction_ == kForward && status_.ok() && - comparator_->Compare(target, key()) >= 0) { - is_increasing_reseek = true; - } ClearHeaps(); status_ = Status::OK(); for (auto& child : children_) { - // If upper bound never changes, we can skip Seek() for - // the !Valid() case too, but people do hack the code to change - // upper bound between Seek(), so it's not a good idea to break - // the API. - // If DBIter is used on top of merging iterator, we probably - // can skip mutable child iterators if they are invalid too, - // but it's a less clean API. We can optimize for it later if - // needed. - if (!is_increasing_reseek || !child.Valid() || - comparator_->Compare(target, child.key()) > 0 || - child.iter()->is_mutable()) { - PERF_TIMER_GUARD(seek_child_seek_time); - child.Seek(target); - PERF_COUNTER_ADD(seek_child_seek_count, 1); - } + PERF_TIMER_GUARD(seek_child_seek_time); + child.Seek(target); + PERF_COUNTER_ADD(seek_child_seek_count, 1); if (child.Valid()) { assert(child.status().ok());