From 712bc4b6a21f1732cc8ee64a1b617184cdeccb84 Mon Sep 17 00:00:00 2001 From: sdong Date: Tue, 17 Mar 2020 12:28:46 -0700 Subject: [PATCH] Fix regression bug in partitioned index reseek caused by #6531 (#6551) Summary: https://github.com/facebook/rocksdb/pull/6531 removed some code in partitioned index seek logic. By mistake the logic of storing previous index offset is removed, while the logic of using it is preserved, so that the code might use wrong value to determine reseeking condition. This will trigger a bug, if following a Seek() not going to the last block, SeekToLast() is called, and then Seek() is called which should position the cursor to the block before SeekToLast(). Pull Request resolved: https://github.com/facebook/rocksdb/pull/6551 Test Plan: Add a unit test that reproduces the bug. In the same unit test, also some reseek cases are covered to avoid regression. Reviewed By: pdillinger Differential Revision: D20493990 fbshipit-source-id: 3919aa4861c0481ec96844e053048da1a934b91d --- .../block_based/partitioned_index_iterator.cc | 10 +-- table/table_test.cc | 79 ++++++++++++++++++- 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/table/block_based/partitioned_index_iterator.cc b/table/block_based/partitioned_index_iterator.cc index 2325ea7bd..b9d527c9e 100644 --- a/table/block_based/partitioned_index_iterator.cc +++ b/table/block_based/partitioned_index_iterator.cc @@ -14,6 +14,8 @@ void ParititionedIndexIterator::Seek(const Slice& target) { SeekImpl(&target); } void ParititionedIndexIterator::SeekToFirst() { SeekImpl(nullptr); } void ParititionedIndexIterator::SeekImpl(const Slice* target) { + SavePrevIndexValue(); + if (target) { index_iter_->Seek(*target); } else { @@ -25,13 +27,7 @@ void ParititionedIndexIterator::SeekImpl(const Slice* target) { return; } - IndexValue v = index_iter_->value(); - const bool same_block = block_iter_points_to_real_block_ && - v.handle.offset() == prev_block_offset_; - - if (!same_block) { - InitPartitionedIndexBlock(); - } + InitPartitionedIndexBlock(); if (target) { block_iter_.Seek(*target); diff --git a/table/table_test.cc b/table/table_test.cc index 72592bc79..e679f6c6e 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -1939,8 +1939,9 @@ void TableTest::IndexTest(BlockBasedTableOptions table_options) { // -- Find keys do not exist, but have common prefix. std::vector prefixes = {"001", "003", "005", "007", "009"}; - std::vector lower_bound = {keys[0], keys[1], keys[2], - keys[7], keys[9], }; + std::vector lower_bound = { + keys[0], keys[1], keys[2], keys[7], keys[9], + }; // find the lower bound of the prefix for (size_t i = 0; i < prefixes.size(); ++i) { @@ -2017,6 +2018,80 @@ void TableTest::IndexTest(BlockBasedTableOptions table_options) { ASSERT_TRUE(BytewiseComparator()->Compare(prefix, ukey_prefix) > 0); } } + + { + // Test reseek case. It should impact partitioned index more. + ReadOptions ro; + ro.total_order_seek = true; + std::unique_ptr index_iter2(reader->NewIterator( + ro, moptions.prefix_extractor.get(), /*arena=*/nullptr, + /*skip_filters=*/false, TableReaderCaller::kUncategorized)); + + // Things to cover in partitioned index: + // 1. Both of Seek() and SeekToLast() has optimization to prevent + // rereek leaf index block if it remains to the same one, and + // they reuse the same variable. + // 2. When Next() or Prev() is called, the block moves, so the + // optimization should kick in only with the current one. + index_iter2->Seek(InternalKey("0055", 0, kTypeValue).Encode()); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0055", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->SeekToLast(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->Seek(InternalKey("0055", 0, kTypeValue).Encode()); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0055", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->SeekToLast(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + index_iter2->Prev(); + ASSERT_TRUE(index_iter2->Valid()); + index_iter2->Prev(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0075", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->Seek(InternalKey("0095", 0, kTypeValue).Encode()); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + index_iter2->Prev(); + ASSERT_TRUE(index_iter2->Valid()); + index_iter2->Prev(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0075", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->SeekToLast(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->Seek(InternalKey("0095", 0, kTypeValue).Encode()); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->Prev(); + ASSERT_TRUE(index_iter2->Valid()); + index_iter2->Prev(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0075", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->Seek(InternalKey("0075", 0, kTypeValue).Encode()); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0075", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->Next(); + ASSERT_TRUE(index_iter2->Valid()); + index_iter2->Next(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + + index_iter2->SeekToLast(); + ASSERT_TRUE(index_iter2->Valid()); + ASSERT_EQ("0095", index_iter2->key().ToString().substr(0, 4)); + } + c.ResetTableReader(); }