diff --git a/HISTORY.md b/HISTORY.md index 559f942c1..290b613d9 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -11,7 +11,6 @@ * DB::CompactRange()'s parameter reduce_level is changed to change_level, to allow users to move levels to lower levels if allowed. It can be used to migrate a DB from options.level_compaction_dynamic_level_bytes=false to options.level_compaction_dynamic_level_bytes.true. * Change default value for options.compaction_filter_factory and options.compaction_filter_factory_v2 to nullptr instead of DefaultCompactionFilterFactory and DefaultCompactionFilterFactoryV2. * If CancelAllBackgroundWork is called without doing a flush after doing loads with WAL disabled, the changes which haven't been flushed before the call to CancelAllBackgroundWork will be lost. -* WBWIIterator::Entry() now returns WriteEntry instead of `const WriteEntry&` ## 3.11.0 (5/19/2015) ### New Features diff --git a/include/rocksdb/utilities/write_batch_with_index.h b/include/rocksdb/utilities/write_batch_with_index.h index 1da4421f3..7c17534aa 100644 --- a/include/rocksdb/utilities/write_batch_with_index.h +++ b/include/rocksdb/utilities/write_batch_with_index.h @@ -55,9 +55,7 @@ class WBWIIterator { virtual void Prev() = 0; - // the return WriteEntry is only valid until the next mutation of - // WriteBatchWithIndex - virtual WriteEntry Entry() const = 0; + virtual const WriteEntry& Entry() const = 0; virtual Status status() const = 0; }; diff --git a/utilities/write_batch_with_index/write_batch_with_index.cc b/utilities/write_batch_with_index/write_batch_with_index.cc index 451ed98ac..0c3e02f3e 100644 --- a/utilities/write_batch_with_index/write_batch_with_index.cc +++ b/utilities/write_batch_with_index/write_batch_with_index.cc @@ -89,8 +89,7 @@ class BaseDeltaIterator : public Iterator { AdvanceBase(); } if (DeltaValid() && BaseValid()) { - if (comparator_->Compare(delta_iterator_->Entry().key, - base_iterator_->key()) == 0) { + if (Compare() == 0) { equal_keys_ = true; } } @@ -124,8 +123,7 @@ class BaseDeltaIterator : public Iterator { AdvanceBase(); } if (DeltaValid() && BaseValid()) { - if (comparator_->Compare(delta_iterator_->Entry().key, - base_iterator_->key()) == 0) { + if (Compare() == 0) { equal_keys_ = true; } } @@ -155,6 +153,23 @@ class BaseDeltaIterator : public Iterator { } private: + // -1 -- delta less advanced than base + // 0 -- delta == base + // 1 -- delta more advanced than base + int Compare() const { + assert(delta_iterator_->Valid() && base_iterator_->Valid()); + int cmp = comparator_->Compare(delta_iterator_->Entry().key, + base_iterator_->key()); + if (forward_) { + return cmp; + } else { + return -cmp; + } + } + bool IsDeltaDelete() { + assert(DeltaValid()); + return delta_iterator_->Entry().type == kDeleteRecord; + } void AssertInvariants() { #ifndef NDEBUG if (!Valid()) { @@ -224,10 +239,6 @@ class BaseDeltaIterator : public Iterator { bool DeltaValid() const { return delta_iterator_->Valid(); } void UpdateCurrent() { while (true) { - WriteEntry delta_entry; - if (DeltaValid()) { - delta_entry = delta_iterator_->Entry(); - } equal_keys_ = false; if (!BaseValid()) { // Base has finished. @@ -235,7 +246,7 @@ class BaseDeltaIterator : public Iterator { // Finished return; } - if (delta_entry.type == kDeleteRecord) { + if (IsDeltaDelete()) { AdvanceDelta(); } else { current_at_base_ = false; @@ -246,14 +257,12 @@ class BaseDeltaIterator : public Iterator { current_at_base_ = true; return; } else { - int compare = - (forward_ ? 1 : -1) * - comparator_->Compare(delta_entry.key, base_iterator_->key()); + int compare = Compare(); if (compare <= 0) { // delta bigger or equal if (compare == 0) { equal_keys_ = true; } - if (delta_entry.type == kDeleteRecord) { + if (!IsDeltaDelete()) { current_at_base_ = false; return; } @@ -291,26 +300,23 @@ class WBWIIteratorImpl : public WBWIIterator { const ReadableWriteBatch* write_batch) : column_family_id_(column_family_id), skip_list_iter_(skip_list), - write_batch_(write_batch) {} + write_batch_(write_batch), + valid_(false) {} virtual ~WBWIIteratorImpl() {} - virtual bool Valid() const override { - if (!skip_list_iter_.Valid()) { - return false; - } - const WriteBatchIndexEntry* iter_entry = skip_list_iter_.key(); - return (iter_entry != nullptr && - iter_entry->column_family == column_family_id_); - } + virtual bool Valid() const override { return valid_; } virtual void SeekToFirst() override { + valid_ = true; WriteBatchIndexEntry search_entry(WriteBatchIndexEntry::kFlagMin, column_family_id_); skip_list_iter_.Seek(&search_entry); + ReadEntry(); } virtual void SeekToLast() override { + valid_ = true; WriteBatchIndexEntry search_entry(WriteBatchIndexEntry::kFlagMin, column_family_id_ + 1); skip_list_iter_.Seek(&search_entry); @@ -319,38 +325,30 @@ class WBWIIteratorImpl : public WBWIIterator { } else { skip_list_iter_.Prev(); } + ReadEntry(); } virtual void Seek(const Slice& key) override { + valid_ = true; WriteBatchIndexEntry search_entry(&key, column_family_id_); skip_list_iter_.Seek(&search_entry); + ReadEntry(); } - virtual void Next() override { skip_list_iter_.Next(); } - - virtual void Prev() override { skip_list_iter_.Prev(); } - - virtual WriteEntry Entry() const override { - WriteEntry ret; - Slice blob; - const WriteBatchIndexEntry* iter_entry = skip_list_iter_.key(); - // this is guaranteed with Valid() - assert(iter_entry != nullptr && - iter_entry->column_family == column_family_id_); - auto s = write_batch_->GetEntryFromDataOffset(iter_entry->offset, &ret.type, - &ret.key, &ret.value, &blob); - assert(s.ok()); - assert(ret.type == kPutRecord || ret.type == kDeleteRecord || - ret.type == kMergeRecord); - return ret; + virtual void Next() override { + skip_list_iter_.Next(); + ReadEntry(); } - virtual Status status() const override { - // this is in-memory data structure, so the only way status can be non-ok is - // through memory corruption - return Status::OK(); + virtual void Prev() override { + skip_list_iter_.Prev(); + ReadEntry(); } + virtual const WriteEntry& Entry() const override { return current_; } + + virtual Status status() const override { return status_; } + const WriteBatchIndexEntry* GetRawEntry() const { return skip_list_iter_.key(); } @@ -359,6 +357,33 @@ class WBWIIteratorImpl : public WBWIIterator { uint32_t column_family_id_; WriteBatchEntrySkipList::Iterator skip_list_iter_; const ReadableWriteBatch* write_batch_; + Status status_; + bool valid_; + WriteEntry current_; + + void ReadEntry() { + if (!status_.ok() || !skip_list_iter_.Valid()) { + valid_ = false; + return; + } + const WriteBatchIndexEntry* iter_entry = skip_list_iter_.key(); + if (iter_entry == nullptr || + iter_entry->column_family != column_family_id_) { + valid_ = false; + return; + } + Slice blob; + status_ = write_batch_->GetEntryFromDataOffset( + iter_entry->offset, ¤t_.type, ¤t_.key, ¤t_.value, + &blob); + if (!status_.ok()) { + valid_ = false; + } else if (current_.type != kPutRecord && current_.type != kDeleteRecord && + current_.type != kMergeRecord) { + valid_ = false; + status_ = Status::Corruption("write batch index is corrupted"); + } + } }; struct WriteBatchWithIndex::Rep { diff --git a/utilities/write_batch_with_index/write_batch_with_index_test.cc b/utilities/write_batch_with_index/write_batch_with_index_test.cc index df4995578..d324eba85 100644 --- a/utilities/write_batch_with_index/write_batch_with_index_test.cc +++ b/utilities/write_batch_with_index/write_batch_with_index_test.cc @@ -103,7 +103,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, std::unique_ptr iter(batch->NewIterator(&data)); iter->Seek(e.key); ASSERT_OK(iter->status()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(e.key, write_entry.key.ToString()); ASSERT_EQ(e.value, write_entry.value.ToString()); batch->Delete(&data, e.key); @@ -124,7 +124,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, for (auto v : pair.second) { ASSERT_OK(iter->status()); ASSERT_TRUE(iter->Valid()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(pair.first, write_entry.key.ToString()); ASSERT_EQ(v->type, write_entry.type); if (write_entry.type != kDeleteRecord) { @@ -140,7 +140,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, for (auto v = pair->second.rbegin(); v != pair->second.rend(); v++) { ASSERT_OK(iter->status()); ASSERT_TRUE(iter->Valid()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(pair->first, write_entry.key.ToString()); ASSERT_EQ((*v)->type, write_entry.type); if (write_entry.type != kDeleteRecord) { @@ -165,7 +165,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, for (auto v : pair.second) { ASSERT_OK(iter->status()); ASSERT_TRUE(iter->Valid()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(pair.first, write_entry.key.ToString()); if (v->type != kDeleteRecord) { ASSERT_EQ(v->key, write_entry.value.ToString()); @@ -182,7 +182,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, for (auto v = pair->second.rbegin(); v != pair->second.rend(); v++) { ASSERT_OK(iter->status()); ASSERT_TRUE(iter->Valid()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(pair->first, write_entry.key.ToString()); if ((*v)->type != kDeleteRecord) { ASSERT_EQ((*v)->key, write_entry.value.ToString()); @@ -204,7 +204,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, ASSERT_OK(iter->status()); for (auto v : pair->second) { ASSERT_TRUE(iter->Valid()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(pair->first, write_entry.key.ToString()); ASSERT_EQ(v->type, write_entry.type); if (write_entry.type != kDeleteRecord) { @@ -226,7 +226,7 @@ void TestValueAsSecondaryIndexHelper(std::vector entries, ASSERT_OK(iter->status()); for (auto v : pair->second) { ASSERT_TRUE(iter->Valid()); - auto write_entry = iter->Entry(); + auto& write_entry = iter->Entry(); ASSERT_EQ(pair->first, write_entry.key.ToString()); ASSERT_EQ(v->value, write_entry.key.ToString()); if (v->type != kDeleteRecord) { @@ -1268,6 +1268,9 @@ TEST_F(WriteBatchWithIndexTest, MutateWhileIteratingBaseCorrectnessTest) { AssertIterKey("mm", iter.get()); AssertIterValue("kk", iter.get()); batch.Delete("mm"); + // still mm even though it's deleted + AssertIterKey("mm", iter.get()); + AssertIterValue("kk", iter.get()); iter->Next(); AssertIterKey("n", iter.get()); iter->Prev();