From 65428b0c0a0f541a08b0e3887c070b7fd6fbc6dc Mon Sep 17 00:00:00 2001
From: Siying Dong <sdong@fb.com>
Date: Thu, 17 Oct 2013 18:33:18 -0700
Subject: [PATCH] Fix Bug: iterator.Prev() or iterator.SeekToLast() might
 return the first element instead of the correct one

Summary:
Recent patch https://reviews.facebook.net/D11865 introduced a regression bug:

DBIter::FindPrevUserEntry(), which is called by DBIter::Prev() (and also implicitly if calling iterator.SeekToLast())  might do issue a seek when having skipped too many entries. If the skipped entry just before the seek() is a delete, the saved key is erased so that it seeks to the front, so Prev() would return the first element.

This patch fixes the bug by not doing seek() in DBIter::FindNextUserEntry() if saved key has been erased.

Test Plan: Add a test DBTest.IterPrevMaxSkip which would fail without the patch and would pass with the change.

Reviewers: dhruba, xjin, haobo

Reviewed By: dhruba

CC: leveldb

Differential Revision: https://reviews.facebook.net/D13557
---
 db/db_iter.cc |  4 +++-
 db/db_test.cc | 43 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/db/db_iter.cc b/db/db_iter.cc
index 8ef9b3ce3..4e3c52c6e 100644
--- a/db/db_iter.cc
+++ b/db/db_iter.cc
@@ -373,6 +373,7 @@ void DBIter::FindPrevUserEntry() {
   if (iter_->Valid()) {
     do {
       ParsedInternalKey ikey;
+      bool saved_key_cleared = false;
       if (ParseKey(&ikey) && ikey.sequence <= sequence_) {
         if ((value_type != kTypeDeletion) &&
             user_comparator_->Compare(ikey.user_key, saved_key_) < 0) {
@@ -383,6 +384,7 @@ void DBIter::FindPrevUserEntry() {
         if (value_type == kTypeDeletion) {
           saved_key_.clear();
           ClearSavedValue();
+          saved_key_cleared = true;
         } else {
           Slice raw_value = iter_->value();
           if (saved_value_.capacity() > raw_value.size() + 1048576) {
@@ -398,7 +400,7 @@ void DBIter::FindPrevUserEntry() {
       // found the prev user-key, then it is better to seek so that we can
       // avoid too many key comparisons. We seek to the first occurence of
       // our current key by looking for max sequence number.
-      if (num_skipped > max_skip_) {
+      if (!saved_key_cleared && num_skipped > max_skip_) {
         num_skipped = 0;
         std::string last_key;
         AppendInternalKey(&last_key,
diff --git a/db/db_test.cc b/db/db_test.cc
index 6695f5f3c..074a902e9 100644
--- a/db/db_test.cc
+++ b/db/db_test.cc
@@ -254,7 +254,8 @@ class DBTest {
   enum OptionSkip {
     kNoSkip = 0,
     kSkipDeletesFilterFirst = 1,
-    kSkipUniversalCompaction = 2
+    kSkipUniversalCompaction = 2,
+    kSkipMergePut = 4
   };
 
   DBTest() : option_config_(kDefault),
@@ -287,7 +288,9 @@ class DBTest {
         option_config_ == kUniversalCompaction) {
       option_config_++;
     }
-
+    if (skip_mask & kSkipMergePut && option_config_ == kMergePut) {
+      option_config_++;
+    }
     if (option_config_ >= kEnd) {
       Destroy(&last_options_);
       return false;
@@ -639,6 +642,13 @@ class DBTest {
   std::string DummyString(size_t len, char c = 'a') {
     return std::string(len, c);
   }
+
+  void VerifyIterLast(std::string expected_key) {
+    Iterator* iter = db_->NewIterator(ReadOptions());
+    iter->SeekToLast();
+    ASSERT_EQ(IterStatus(iter), expected_key);
+    delete iter;
+  }
 };
 
 TEST(DBTest, Empty) {
@@ -1314,6 +1324,35 @@ TEST(DBTest, IterMultiWithDelete) {
   } while (ChangeOptions());
 }
 
+TEST(DBTest, IterPrevMaxSkip) {
+  do {
+    for (int i = 0; i < 2; i++) {
+      db_->Put(WriteOptions(), "key1", "v1");
+      db_->Put(WriteOptions(), "key2", "v2");
+      db_->Put(WriteOptions(), "key3", "v3");
+      db_->Put(WriteOptions(), "key4", "v4");
+      db_->Put(WriteOptions(), "key5", "v5");
+    }
+
+    VerifyIterLast("key5->v5");
+
+    ASSERT_OK(db_->Delete(WriteOptions(), "key5"));
+    VerifyIterLast("key4->v4");
+
+    ASSERT_OK(db_->Delete(WriteOptions(), "key4"));
+    VerifyIterLast("key3->v3");
+
+    ASSERT_OK(db_->Delete(WriteOptions(), "key3"));
+    VerifyIterLast("key2->v2");
+
+    ASSERT_OK(db_->Delete(WriteOptions(), "key2"));
+    VerifyIterLast("key1->v1");
+
+    ASSERT_OK(db_->Delete(WriteOptions(), "key1"));
+    VerifyIterLast("(invalid)");
+  } while (ChangeOptions(kSkipMergePut));
+}
+
 TEST(DBTest, IterWithSnapshot) {
   do {
     ASSERT_OK(Put("key1", "val1"));