From 2a81633da27ddd0cc3d3799938d456c0f6284cd4 Mon Sep 17 00:00:00 2001
From: Siying Dong <siying.d@fb.com>
Date: Wed, 25 Jul 2018 17:03:39 -0700
Subject: [PATCH] Fix bug when seeking backward against an out-of-bound
 iterator (#4187)

Summary:
92ee3350e0ae02c0973af0fbd40fb67b0b958128 introduces an out-of-bound check in BlockBasedTableIterator::Valid(). However, this flag is not reset when re-seeking in backward direction. This caused the iterator to be invalide by mistake. Fix it by always resetting the out-of-bound flag in every seek.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/4187

Differential Revision: D8996600

Pulled By: siying

fbshipit-source-id: b6235ea614f71381e50e7904c4fb036300604ac1
---
 db/db_iterator_test.cc            | 24 ++++++++++++++++++++++++
 table/block_based_table_reader.cc |  7 ++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc
index 63a11752e9..9df7bfbbce 100644
--- a/db/db_iterator_test.cc
+++ b/db/db_iterator_test.cc
@@ -2413,6 +2413,30 @@ TEST_P(DBIteratorTest, NonBlockingIterationBugRepro) {
   EXPECT_TRUE(iter->status().IsIncomplete());
 }
 
+TEST_P(DBIteratorTest, SeekBackwardAfterOutOfUpperBound) {
+  Put("a", "");
+  Put("b", "");
+  Flush();
+
+  ReadOptions ropt;
+  Slice ub = "b";
+  ropt.iterate_upper_bound = &ub;
+
+  std::unique_ptr<Iterator> it(dbfull()->NewIterator(ropt));
+  it->SeekForPrev("a");
+  ASSERT_TRUE(it->Valid());
+  ASSERT_OK(it->status());
+  ASSERT_EQ("a", it->key().ToString());
+  it->Next();
+  ASSERT_FALSE(it->Valid());
+  ASSERT_OK(it->status());
+  it->SeekForPrev("a");
+  ASSERT_OK(it->status());
+
+  ASSERT_TRUE(it->Valid());
+  ASSERT_EQ("a", it->key().ToString());
+}
+
 INSTANTIATE_TEST_CASE_P(DBIteratorTestInstance, DBIteratorTest,
                         testing::Values(true, false));
 
diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc
index 8b83cda53f..e2d92529bd 100644
--- a/table/block_based_table_reader.cc
+++ b/table/block_based_table_reader.cc
@@ -1991,6 +1991,7 @@ bool BlockBasedTable::PrefixMayMatch(
 
 template <class TBlockIter>
 void BlockBasedTableIterator<TBlockIter>::Seek(const Slice& target) {
+  is_out_of_bound_ = false;
   if (!CheckPrefixMayMatch(target)) {
     ResetDataIter();
     return;
@@ -2020,6 +2021,7 @@ void BlockBasedTableIterator<TBlockIter>::Seek(const Slice& target) {
 
 template <class TBlockIter>
 void BlockBasedTableIterator<TBlockIter>::SeekForPrev(const Slice& target) {
+  is_out_of_bound_ = false;
   if (!CheckPrefixMayMatch(target)) {
     ResetDataIter();
     return;
@@ -2062,6 +2064,7 @@ void BlockBasedTableIterator<TBlockIter>::SeekForPrev(const Slice& target) {
 
 template <class TBlockIter>
 void BlockBasedTableIterator<TBlockIter>::SeekToFirst() {
+  is_out_of_bound_ = false;
   SavePrevIndexValue();
   index_iter_->SeekToFirst();
   if (!index_iter_->Valid()) {
@@ -2075,6 +2078,7 @@ void BlockBasedTableIterator<TBlockIter>::SeekToFirst() {
 
 template <class TBlockIter>
 void BlockBasedTableIterator<TBlockIter>::SeekToLast() {
+  is_out_of_bound_ = false;
   SavePrevIndexValue();
   index_iter_->SeekToLast();
   if (!index_iter_->Valid()) {
@@ -2153,7 +2157,7 @@ void BlockBasedTableIterator<TBlockIter>::InitDataBlock() {
 
 template <class TBlockIter>
 void BlockBasedTableIterator<TBlockIter>::FindKeyForward() {
-  is_out_of_bound_ = false;
+  assert(!is_out_of_bound_);
   // TODO the while loop inherits from two-level-iterator. We don't know
   // whether a block can be empty so it can be replaced by an "if".
   while (!block_iter_.Valid()) {
@@ -2193,6 +2197,7 @@ void BlockBasedTableIterator<TBlockIter>::FindKeyForward() {
 
 template <class TBlockIter>
 void BlockBasedTableIterator<TBlockIter>::FindKeyBackward() {
+  assert(!is_out_of_bound_);
   while (!block_iter_.Valid()) {
     if (!block_iter_.status().ok()) {
       return;