From d82f1421b44dd84f77de8990d1422fa68f9021f0 Mon Sep 17 00:00:00 2001 From: Siying Dong Date: Tue, 12 Jun 2018 16:43:44 -0700 Subject: [PATCH] Fix regression bug of Prev() with upper bound (#3989) Summary: A recent change pushed down the upper bound checking to child iterators. However, this causes the logic of following sequence wrong: Seek(key); if (!Valid()) SeekToLast(); Because !Valid() may be caused by upper bounds, rather than the end of the iterator. In this case SeekToLast() points to totally wrong places. This can cause wrong results, infinite loops, or segfault in some cases. This sequence is called when changing direction from forward to backward. And this by itself also implicitly happen during reseeking optimization in Prev(). Fix this bug by using SeekForPrev() rather than this sequuence, as what is already done in prefix extrator case. Closes https://github.com/facebook/rocksdb/pull/3989 Differential Revision: D8385422 Pulled By: siying fbshipit-source-id: 429e869990cfd2dc389421e0836fc496bed67bb4 --- db/db_iter_test.cc | 10 +++-- db/db_iterator_test.cc | 78 +++++++++++++++++++++++++++++++++++++++ table/merging_iterator.cc | 26 +++---------- 3 files changed, 89 insertions(+), 25 deletions(-) diff --git a/db/db_iter_test.cc b/db/db_iter_test.cc index aa4b39c38..b1f5e43a5 100644 --- a/db/db_iter_test.cc +++ b/db/db_iter_test.cc @@ -36,7 +36,9 @@ class TestIterator : public InternalIterator { valid_(false), sequence_number_(0), iter_(0), - cmp(comparator) {} + cmp(comparator) { + data_.reserve(16); + } void AddPut(std::string argkey, std::string argvalue) { Add(argkey, kTypeValue, argvalue); @@ -2660,7 +2662,7 @@ TEST_F(DBIterWithMergeIterTest, InnerMergeIteratorDataRace1) { // MergeIterator::Prev() realized the mem table iterator is at its end // and before an SeekToLast() is called. rocksdb::SyncPoint::GetInstance()->SetCallBack( - "MergeIterator::Prev:BeforeSeekToLast", + "MergeIterator::Prev:BeforePrev", [&](void* /*arg*/) { internal_iter2_->Add("z", kTypeValue, "7", 12u); }); rocksdb::SyncPoint::GetInstance()->EnableProcessing(); @@ -2696,7 +2698,7 @@ TEST_F(DBIterWithMergeIterTest, InnerMergeIteratorDataRace2) { // mem table after MergeIterator::Prev() realized the mem tableiterator is at // its end and before an SeekToLast() is called. rocksdb::SyncPoint::GetInstance()->SetCallBack( - "MergeIterator::Prev:BeforeSeekToLast", [&](void* /*arg*/) { + "MergeIterator::Prev:BeforePrev", [&](void* /*arg*/) { internal_iter2_->Add("z", kTypeValue, "7", 12u); internal_iter2_->Add("z", kTypeValue, "7", 11u); }); @@ -2734,7 +2736,7 @@ TEST_F(DBIterWithMergeIterTest, InnerMergeIteratorDataRace3) { // mem table after MergeIterator::Prev() realized the mem table iterator is at // its end and before an SeekToLast() is called. rocksdb::SyncPoint::GetInstance()->SetCallBack( - "MergeIterator::Prev:BeforeSeekToLast", [&](void* /*arg*/) { + "MergeIterator::Prev:BeforePrev", [&](void* /*arg*/) { internal_iter2_->Add("z", kTypeValue, "7", 16u, true); internal_iter2_->Add("z", kTypeValue, "7", 15u, true); internal_iter2_->Add("z", kTypeValue, "7", 14u, true); diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index e6504dc50..a7b389cf3 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -2044,6 +2044,43 @@ TEST_P(DBIteratorTest, CreationFailure) { delete iter; } +TEST_P(DBIteratorTest, UpperBoundWithChangeDirection) { + Options options = CurrentOptions(); + options.max_sequential_skip_in_iterations = 3; + DestroyAndReopen(options); + + // write a bunch of kvs to the database. + ASSERT_OK(Put("a", "1")); + ASSERT_OK(Put("y", "1")); + ASSERT_OK(Put("y1", "1")); + ASSERT_OK(Put("y2", "1")); + ASSERT_OK(Put("y3", "1")); + ASSERT_OK(Put("z", "1")); + ASSERT_OK(Flush()); + ASSERT_OK(Put("a", "1")); + ASSERT_OK(Put("z", "1")); + ASSERT_OK(Put("bar", "1")); + ASSERT_OK(Put("foo", "1")); + + std::string upper_bound = "x"; + Slice ub_slice(upper_bound); + ReadOptions ro; + ro.iterate_upper_bound = &ub_slice; + ro.max_skippable_internal_keys = 1000; + + Iterator* iter = NewIterator(ro); + iter->Seek("foo"); + ASSERT_TRUE(iter->Valid()); + ASSERT_EQ("foo", iter->key().ToString()); + + iter->Prev(); + ASSERT_TRUE(iter->Valid()); + ASSERT_OK(iter->status()); + ASSERT_EQ("bar", iter->key().ToString()); + + delete iter; +} + TEST_P(DBIteratorTest, TableFilter) { ASSERT_OK(Put("a", "1")); dbfull()->Flush(FlushOptions()); @@ -2110,6 +2147,47 @@ TEST_P(DBIteratorTest, TableFilter) { } } +TEST_P(DBIteratorTest, UpperBoundWithPrevReseek) { + Options options = CurrentOptions(); + options.max_sequential_skip_in_iterations = 3; + DestroyAndReopen(options); + + // write a bunch of kvs to the database. + ASSERT_OK(Put("a", "1")); + ASSERT_OK(Put("y", "1")); + ASSERT_OK(Put("z", "1")); + ASSERT_OK(Flush()); + ASSERT_OK(Put("a", "1")); + ASSERT_OK(Put("z", "1")); + ASSERT_OK(Put("bar", "1")); + ASSERT_OK(Put("foo", "1")); + ASSERT_OK(Put("foo", "2")); + + ASSERT_OK(Put("foo", "3")); + ASSERT_OK(Put("foo", "4")); + ASSERT_OK(Put("foo", "5")); + const Snapshot* snapshot = db_->GetSnapshot(); + ASSERT_OK(Put("foo", "6")); + + std::string upper_bound = "x"; + Slice ub_slice(upper_bound); + ReadOptions ro; + ro.snapshot = snapshot; + ro.iterate_upper_bound = &ub_slice; + + Iterator* iter = NewIterator(ro); + iter->SeekForPrev("goo"); + ASSERT_TRUE(iter->Valid()); + ASSERT_EQ("foo", iter->key().ToString()); + iter->Prev(); + + ASSERT_TRUE(iter->Valid()); + ASSERT_EQ("bar", iter->key().ToString()); + + delete iter; + db_->ReleaseSnapshot(snapshot); +} + TEST_P(DBIteratorTest, SkipStatistics) { Options options = CurrentOptions(); options.statistics = rocksdb::CreateDBStatistics(); diff --git a/table/merging_iterator.cc b/table/merging_iterator.cc index c2f486c2a..d0bd0f836 100644 --- a/table/merging_iterator.cc +++ b/table/merging_iterator.cc @@ -228,28 +228,12 @@ class MergingIterator : public InternalIterator { Slice target = key(); for (auto& child : children_) { if (&child != current_) { - if (!prefix_seek_mode_) { - child.Seek(target); - if (child.Valid()) { - // Child is at first entry >= key(). Step back one to be < key() - TEST_SYNC_POINT_CALLBACK("MergeIterator::Prev:BeforePrev", - &child); - assert(child.status().ok()); - child.Prev(); - } else { - // Child has no entries >= key(). Position at last entry. - TEST_SYNC_POINT("MergeIterator::Prev:BeforeSeekToLast"); - considerStatus(child.status()); - child.SeekToLast(); - } + child.SeekForPrev(target); + TEST_SYNC_POINT_CALLBACK("MergeIterator::Prev:BeforePrev", &child); + considerStatus(child.status()); + if (child.Valid() && comparator_->Equal(target, child.key())) { + child.Prev(); considerStatus(child.status()); - } else { - child.SeekForPrev(target); - considerStatus(child.status()); - if (child.Valid() && comparator_->Equal(target, child.key())) { - child.Prev(); - considerStatus(child.status()); - } } } if (child.Valid()) {