From 21e8daced5a2db9ba55138c60a320676306f2088 Mon Sep 17 00:00:00 2001 From: Aaron Gao Date: Thu, 13 Oct 2016 17:36:48 -0700 Subject: [PATCH] fix assertion failure in Prev() Summary: fix assertion failure in db_stress. It happens because of prefix seek key is larger than merge iterator key when they have the same user key Test Plan: ./db_stress --max_background_compactions=1 --max_write_buffer_number=3 --sync=0 --reopen=20 --write_buffer_size=33554432 --delpercent=5 --log2_keys_per_lock=10 --block_size=16384 --allow_concurrent_memtable_write=0 --test_batches_snapshots=0 --max_bytes_for_level_base=67108864 --progress_reports=0 --mmap_read=0 --writepercent=35 --disable_data_sync=0 --readpercent=50 --subcompactions=4 --ops_per_thread=20000000 --memtablerep=skip_list --prefix_size=0 --target_file_size_multiplier=1 --column_families=1 --threads=32 --disable_wal=0 --open_files=500000 --destroy_db_initially=0 --target_file_size_base=16777216 --nooverwritepercent=1 --iterpercent=10 --max_key=100000000 --prefixpercent=0 --use_clock_cache=false --kill_random_test=888887 --cache_size=1048576 --verify_checksum=1 Reviewers: sdong, andrewkr, yiwu, yhchiang Reviewed By: yhchiang Subscribers: andrewkr, dhruba, leveldb Differential Revision: https://reviews.facebook.net/D65025 --- db/db_iter.cc | 6 ++---- db/db_iterator_test.cc | 7 ++++--- table/internal_iterator.h | 6 ------ table/merger.cc | 16 ++-------------- 4 files changed, 8 insertions(+), 27 deletions(-) diff --git a/db/db_iter.cc b/db/db_iter.cc index afc4596a5..0c4f2b685 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -512,8 +512,7 @@ void DBIter::ReverseToForward() { IterKey last_key; last_key.SetInternalKey(ParsedInternalKey( saved_key_.GetKey(), kMaxSequenceNumber, kValueTypeForSeek)); - Slice db_iter_key = last_key.GetKey(); - iter_->ResetPrefixSeekKey(&db_iter_key); + iter_->Seek(last_key.GetKey()); } FindNextUserKey(); direction_ = kForward; @@ -527,8 +526,7 @@ void DBIter::ReverseToBackward() { IterKey last_key; last_key.SetInternalKey( ParsedInternalKey(saved_key_.GetKey(), 0, kValueTypeForSeekForPrev)); - Slice db_iter_key = last_key.GetKey(); - iter_->ResetPrefixSeekKey(&db_iter_key); + iter_->SeekForPrev(last_key.GetKey()); } if (current_entry_is_merged_) { // Not placed in the same key. Need to call Prev() until finding the diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index e6bb25405..403cdc56a 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -666,7 +666,8 @@ TEST_F(DBIteratorTest, IterMultiWithDelete) { // TODO: merge operator does not support backward iteration yet if (kPlainTableAllBytesPrefix != option_config_ && kBlockBasedTableWithWholeKeyHashIndex != option_config_ && - kHashLinkList != option_config_) { + kHashLinkList != option_config_ && + kHashSkipList != option_config_) { // doesn't support SeekToLast iter->Prev(); ASSERT_EQ(IterStatus(iter), "ka->va"); } @@ -732,7 +733,7 @@ TEST_F(DBIteratorTest, IterWithSnapshot) { // TODO: merge operator does not support backward iteration yet if (kPlainTableAllBytesPrefix != option_config_ && kBlockBasedTableWithWholeKeyHashIndex != option_config_ && - kHashLinkList != option_config_) { + kHashLinkList != option_config_ && kHashSkipList != option_config_) { iter->Prev(); ASSERT_EQ(IterStatus(iter), "key4->val4"); iter->Prev(); @@ -751,7 +752,7 @@ TEST_F(DBIteratorTest, IterWithSnapshot) { // TODO(gzh): merge operator does not support backward iteration yet if (kPlainTableAllBytesPrefix != option_config_ && kBlockBasedTableWithWholeKeyHashIndex != option_config_ && - kHashLinkList != option_config_) { + kHashLinkList != option_config_ && kHashSkipList != option_config_) { iter->SeekForPrev("key1"); ASSERT_EQ(IterStatus(iter), "key1->val1"); iter->Next(); diff --git a/table/internal_iterator.h b/table/internal_iterator.h index 53a4a05ef..62248007c 100644 --- a/table/internal_iterator.h +++ b/table/internal_iterator.h @@ -94,12 +94,6 @@ class InternalIterator : public Cleanable { virtual Status GetProperty(std::string prop_name, std::string* prop) { return Status::NotSupported(""); } - // Reset the key used for Seek() in merge iterator, especially for prefix - // seek mode - // When in prefix_seek_mode, there is inconsistency between db_iterator and - // merge iterator. This inconsistency can cause problem when do Seek() in - // merge iterator in prefix mode. - virtual void ResetPrefixSeekKey(const Slice* prefix_seek_key = nullptr) {} protected: void SeekForPrevImpl(const Slice& target, const Comparator* cmp) { diff --git a/table/merger.cc b/table/merger.cc index 84fae2624..5e8c0d730 100644 --- a/table/merger.cc +++ b/table/merger.cc @@ -158,7 +158,7 @@ class MergingIterator : public InternalIterator { ClearHeaps(); for (auto& child : children_) { if (&child != current_) { - child.Seek(prefix_seek_key_ ? *prefix_seek_key_ : key()); + child.Seek(key()); if (child.Valid() && comparator_->Equal(key(), child.key())) { child.Next(); } @@ -204,7 +204,7 @@ class MergingIterator : public InternalIterator { InitMaxHeap(); for (auto& child : children_) { if (&child != current_) { - child.SeekForPrev(prefix_seek_key_ ? *prefix_seek_key_ : key()); + child.SeekForPrev(key()); if (child.Valid() && comparator_->Equal(key(), child.key())) { child.Prev(); } @@ -277,17 +277,6 @@ class MergingIterator : public InternalIterator { current_->IsValuePinned(); } - virtual void ResetPrefixSeekKey(const Slice* db_iter_key) override { - if (db_iter_key == nullptr) { - prefix_seek_key_.reset(); - return; - } - if (!prefix_seek_key_) { - prefix_seek_key_.reset(new std::string); - } - *prefix_seek_key_ = db_iter_key->ToString(); - } - private: // Clears heaps for both directions, used when changing direction or seeking void ClearHeaps(); @@ -314,7 +303,6 @@ class MergingIterator : public InternalIterator { // forward. Lazily initialize it to save memory. std::unique_ptr maxHeap_; PinnedIteratorsManager* pinned_iters_mgr_; - std::unique_ptr prefix_seek_key_; IteratorWrapper* CurrentForward() const { assert(direction_ == kForward); -- GitLab