dolysis
/
rocksdb
mirror of https://github.com/facebook/rocksdb.git
3
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Siying Dong 2013-10-17 18:33:18 -07:00
parent 9edda37027
commit 65428b0c0a
2 changed files with 44 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
db/db_iter.cc
View File

@ -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,

43
db/db_test.cc
View File

@ -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"));