2016-02-09 23:12:00 +00:00
|
|
|
// Copyright (c) 2011-present, Facebook, Inc. All rights reserved.
|
2017-07-15 23:03:42 +00:00
|
|
|
// This source code is licensed under both the GPLv2 (found in the
|
|
|
|
// COPYING file in the root directory) and Apache 2.0 License
|
|
|
|
// (found in the LICENSE.Apache file in the root directory).
|
2013-10-16 21:59:46 +00:00
|
|
|
//
|
2011-03-18 22:37:00 +00:00
|
|
|
// Copyright (c) 2011 The LevelDB Authors. All rights reserved.
|
|
|
|
// Use of this source code is governed by a BSD-style license that can be
|
|
|
|
// found in the LICENSE file. See the AUTHORS file for names of contributors.
|
|
|
|
|
|
|
|
#include "table/two_level_iterator.h"
|
2016-04-26 19:41:07 +00:00
|
|
|
#include "db/pinned_iterators_manager.h"
|
2013-10-29 00:54:09 +00:00
|
|
|
#include "rocksdb/options.h"
|
|
|
|
#include "rocksdb/table.h"
|
2011-03-18 22:37:00 +00:00
|
|
|
#include "table/block.h"
|
|
|
|
#include "table/format.h"
|
In DB::NewIterator(), try to allocate the whole iterator tree in an arena
Summary:
In this patch, try to allocate the whole iterator tree starting from DBIter from an arena
1. ArenaWrappedDBIter is created when serves as the entry point of an iterator tree, with an arena in it.
2. Add an option to create iterator from arena for following iterators: DBIter, MergingIterator, MemtableIterator, all mem table's iterators, all table reader's iterators and two level iterator.
3. MergeIteratorBuilder is created to incrementally build the tree of internal iterators. It is passed to mem table list and version set and add iterators to it.
Limitations:
(1) Only DB::NewIterator() without tailing uses the arena. Other cases, including readonly DB and compactions are still from malloc
(2) Two level iterator itself is allocated in arena, but not iterators inside it.
Test Plan: make all check
Reviewers: ljin, haobo
Reviewed By: haobo
Subscribers: leveldb, dhruba, yhchiang, igor
Differential Revision: https://reviews.facebook.net/D18513
2014-06-02 23:38:00 +00:00
|
|
|
#include "util/arena.h"
|
2011-03-18 22:37:00 +00:00
|
|
|
|
2013-10-04 04:49:15 +00:00
|
|
|
namespace rocksdb {
|
2011-03-18 22:37:00 +00:00
|
|
|
|
|
|
|
namespace {
|
|
|
|
|
2015-10-12 22:06:38 +00:00
|
|
|
class TwoLevelIterator : public InternalIterator {
|
2011-03-18 22:37:00 +00:00
|
|
|
public:
|
2014-04-25 19:22:23 +00:00
|
|
|
explicit TwoLevelIterator(TwoLevelIteratorState* state,
|
2018-02-13 00:57:56 +00:00
|
|
|
InternalIterator* first_level_iter);
|
2011-03-18 22:37:00 +00:00
|
|
|
|
2018-04-16 00:19:57 +00:00
|
|
|
virtual ~TwoLevelIterator() {
|
|
|
|
first_level_iter_.DeleteIter(false /* is_arena_mode */);
|
|
|
|
second_level_iter_.DeleteIter(false /* is_arena_mode */);
|
|
|
|
delete state_;
|
|
|
|
}
|
2011-03-18 22:37:00 +00:00
|
|
|
|
2015-02-26 19:28:41 +00:00
|
|
|
virtual void Seek(const Slice& target) override;
|
2016-09-28 01:20:57 +00:00
|
|
|
virtual void SeekForPrev(const Slice& target) override;
|
2015-02-26 19:28:41 +00:00
|
|
|
virtual void SeekToFirst() override;
|
|
|
|
virtual void SeekToLast() override;
|
|
|
|
virtual void Next() override;
|
|
|
|
virtual void Prev() override;
|
|
|
|
|
|
|
|
virtual bool Valid() const override { return second_level_iter_.Valid(); }
|
|
|
|
virtual Slice key() const override {
|
2011-03-18 22:37:00 +00:00
|
|
|
assert(Valid());
|
2014-04-25 19:22:23 +00:00
|
|
|
return second_level_iter_.key();
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
2015-02-26 19:28:41 +00:00
|
|
|
virtual Slice value() const override {
|
2011-03-18 22:37:00 +00:00
|
|
|
assert(Valid());
|
2014-04-25 19:22:23 +00:00
|
|
|
return second_level_iter_.value();
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
2015-02-26 19:28:41 +00:00
|
|
|
virtual Status status() const override {
|
2014-04-25 19:22:23 +00:00
|
|
|
if (!first_level_iter_.status().ok()) {
|
Change and clarify the relationship between Valid(), status() and Seek*() for all iterators. Also fix some bugs
Summary:
Before this PR, Iterator/InternalIterator may simultaneously have non-ok status() and Valid() = true. That state means that the last operation failed, but the iterator is nevertheless positioned on some unspecified record. Likely intended uses of that are:
* If some sst files are corrupted, a normal iterator can be used to read the data from files that are not corrupted.
* When using read_tier = kBlockCacheTier, read the data that's in block cache, skipping over the data that is not.
However, this behavior wasn't documented well (and until recently the wiki on github had misleading incorrect information). In the code there's a lot of confusion about the relationship between status() and Valid(), and about whether Seek()/SeekToLast()/etc reset the status or not. There were a number of bugs caused by this confusion, both inside rocksdb and in the code that uses rocksdb (including ours).
This PR changes the convention to:
* If status() is not ok, Valid() always returns false.
* Any seek operation resets status. (Before the PR, it depended on iterator type and on particular error.)
This does sacrifice the two use cases listed above, but siying said it's ok.
Overview of the changes:
* A commit that adds missing status checks in MergingIterator. This fixes a bug that actually affects us, and we need it fixed. `DBIteratorTest.NonBlockingIterationBugRepro` explains the scenario.
* Changes to lots of iterator types to make all of them conform to the new convention. Some bug fixes along the way. By far the biggest changes are in DBIter, which is a big messy piece of code; I tried to make it less big and messy but mostly failed.
* A stress-test for DBIter, to gain some confidence that I didn't break it. It does a few million random operations on the iterator, while occasionally modifying the underlying data (like ForwardIterator does) and occasionally returning non-ok status from internal iterator.
To find the iterator types that needed changes I searched for "public .*Iterator" in the code. Here's an overview of all 27 iterator types:
Iterators that didn't need changes:
* status() is always ok(), or Valid() is always false: MemTableIterator, ModelIter, TestIterator, KVIter (2 classes with this name anonymous namespaces), LoggingForwardVectorIterator, VectorIterator, MockTableIterator, EmptyIterator, EmptyInternalIterator.
* Thin wrappers that always pass through Valid() and status(): ArenaWrappedDBIter, TtlIterator, InternalIteratorFromIterator.
Iterators with changes (see inline comments for details):
* DBIter - an overhaul:
- It used to silently skip corrupted keys (`FindParseableKey()`), which seems dangerous. This PR makes it just stop immediately after encountering a corrupted key, just like it would for other kinds of corruption. Let me know if there was actually some deeper meaning in this behavior and I should put it back.
- It had a few code paths silently discarding subiterator's status. The stress test caught a few.
- The backwards iteration code path was expecting the internal iterator's set of keys to be immutable. It's probably always true in practice at the moment, since ForwardIterator doesn't support backwards iteration, but this PR fixes it anyway. See added DBIteratorTest.ReverseToForwardBug for an example.
- Some parts of backwards iteration code path even did things like `assert(iter_->Valid())` after a seek, which is never a safe assumption.
- It used to not reset status on seek for some types of errors.
- Some simplifications and better comments.
- Some things got more complicated from the added error handling. I'm open to ideas for how to make it nicer.
* MergingIterator - check status after every operation on every subiterator, and in some places assert that valid subiterators have ok status.
* ForwardIterator - changed to the new convention, also slightly simplified.
* ForwardLevelIterator - fixed some bugs and simplified.
* LevelIterator - simplified.
* TwoLevelIterator - changed to the new convention. Also fixed a bug that would make SeekForPrev() sometimes silently ignore errors from first_level_iter_.
* BlockBasedTableIterator - minor changes.
* BlockIter - replaced `SetStatus()` with `Invalidate()` to make sure non-ok BlockIter is always invalid.
* PlainTableIterator - some seeks used to not reset status.
* CuckooTableIterator - tiny code cleanup.
* ManagedIterator - fixed some bugs.
* BaseDeltaIterator - changed to the new convention and fixed a bug.
* BlobDBIterator - seeks used to not reset status.
* KeyConvertingIterator - some small change.
Closes https://github.com/facebook/rocksdb/pull/3810
Differential Revision: D7888019
Pulled By: al13n321
fbshipit-source-id: 4aaf6d3421c545d16722a815b2fa2e7912bc851d
2018-05-17 09:44:14 +00:00
|
|
|
assert(second_level_iter_.iter() == nullptr);
|
2014-04-25 19:22:23 +00:00
|
|
|
return first_level_iter_.status();
|
|
|
|
} else if (second_level_iter_.iter() != nullptr &&
|
|
|
|
!second_level_iter_.status().ok()) {
|
|
|
|
return second_level_iter_.status();
|
2011-03-18 22:37:00 +00:00
|
|
|
} else {
|
|
|
|
return status_;
|
|
|
|
}
|
|
|
|
}
|
2016-04-26 19:41:07 +00:00
|
|
|
virtual void SetPinnedItersMgr(
|
2018-03-05 21:08:17 +00:00
|
|
|
PinnedIteratorsManager* /*pinned_iters_mgr*/) override {}
|
2018-02-13 00:57:56 +00:00
|
|
|
virtual bool IsKeyPinned() const override { return false; }
|
|
|
|
virtual bool IsValuePinned() const override { return false; }
|
2011-03-18 22:37:00 +00:00
|
|
|
|
|
|
|
private:
|
|
|
|
void SaveError(const Status& s) {
|
|
|
|
if (status_.ok() && !s.ok()) status_ = s;
|
|
|
|
}
|
|
|
|
void SkipEmptyDataBlocksForward();
|
|
|
|
void SkipEmptyDataBlocksBackward();
|
2015-10-12 22:06:38 +00:00
|
|
|
void SetSecondLevelIterator(InternalIterator* iter);
|
2011-03-18 22:37:00 +00:00
|
|
|
void InitDataBlock();
|
|
|
|
|
2015-06-30 21:15:24 +00:00
|
|
|
TwoLevelIteratorState* state_;
|
2014-04-25 19:22:23 +00:00
|
|
|
IteratorWrapper first_level_iter_;
|
|
|
|
IteratorWrapper second_level_iter_; // May be nullptr
|
2011-03-18 22:37:00 +00:00
|
|
|
Status status_;
|
2014-04-25 19:22:23 +00:00
|
|
|
// If second_level_iter is non-nullptr, then "data_block_handle_" holds the
|
|
|
|
// "index_value" passed to block_function_ to create the second_level_iter.
|
2011-03-18 22:37:00 +00:00
|
|
|
std::string data_block_handle_;
|
|
|
|
};
|
|
|
|
|
2014-04-25 19:22:23 +00:00
|
|
|
TwoLevelIterator::TwoLevelIterator(TwoLevelIteratorState* state,
|
2018-02-13 00:57:56 +00:00
|
|
|
InternalIterator* first_level_iter)
|
|
|
|
: state_(state), first_level_iter_(first_level_iter) {}
|
2011-03-18 22:37:00 +00:00
|
|
|
|
|
|
|
void TwoLevelIterator::Seek(const Slice& target) {
|
2014-04-25 19:23:07 +00:00
|
|
|
first_level_iter_.Seek(target);
|
|
|
|
|
|
|
|
InitDataBlock();
|
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.Seek(target);
|
2014-04-25 19:22:23 +00:00
|
|
|
}
|
2014-04-25 19:23:07 +00:00
|
|
|
SkipEmptyDataBlocksForward();
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
|
|
|
|
2016-09-28 01:20:57 +00:00
|
|
|
void TwoLevelIterator::SeekForPrev(const Slice& target) {
|
|
|
|
first_level_iter_.Seek(target);
|
|
|
|
InitDataBlock();
|
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.SeekForPrev(target);
|
|
|
|
}
|
|
|
|
if (!Valid()) {
|
Change and clarify the relationship between Valid(), status() and Seek*() for all iterators. Also fix some bugs
Summary:
Before this PR, Iterator/InternalIterator may simultaneously have non-ok status() and Valid() = true. That state means that the last operation failed, but the iterator is nevertheless positioned on some unspecified record. Likely intended uses of that are:
* If some sst files are corrupted, a normal iterator can be used to read the data from files that are not corrupted.
* When using read_tier = kBlockCacheTier, read the data that's in block cache, skipping over the data that is not.
However, this behavior wasn't documented well (and until recently the wiki on github had misleading incorrect information). In the code there's a lot of confusion about the relationship between status() and Valid(), and about whether Seek()/SeekToLast()/etc reset the status or not. There were a number of bugs caused by this confusion, both inside rocksdb and in the code that uses rocksdb (including ours).
This PR changes the convention to:
* If status() is not ok, Valid() always returns false.
* Any seek operation resets status. (Before the PR, it depended on iterator type and on particular error.)
This does sacrifice the two use cases listed above, but siying said it's ok.
Overview of the changes:
* A commit that adds missing status checks in MergingIterator. This fixes a bug that actually affects us, and we need it fixed. `DBIteratorTest.NonBlockingIterationBugRepro` explains the scenario.
* Changes to lots of iterator types to make all of them conform to the new convention. Some bug fixes along the way. By far the biggest changes are in DBIter, which is a big messy piece of code; I tried to make it less big and messy but mostly failed.
* A stress-test for DBIter, to gain some confidence that I didn't break it. It does a few million random operations on the iterator, while occasionally modifying the underlying data (like ForwardIterator does) and occasionally returning non-ok status from internal iterator.
To find the iterator types that needed changes I searched for "public .*Iterator" in the code. Here's an overview of all 27 iterator types:
Iterators that didn't need changes:
* status() is always ok(), or Valid() is always false: MemTableIterator, ModelIter, TestIterator, KVIter (2 classes with this name anonymous namespaces), LoggingForwardVectorIterator, VectorIterator, MockTableIterator, EmptyIterator, EmptyInternalIterator.
* Thin wrappers that always pass through Valid() and status(): ArenaWrappedDBIter, TtlIterator, InternalIteratorFromIterator.
Iterators with changes (see inline comments for details):
* DBIter - an overhaul:
- It used to silently skip corrupted keys (`FindParseableKey()`), which seems dangerous. This PR makes it just stop immediately after encountering a corrupted key, just like it would for other kinds of corruption. Let me know if there was actually some deeper meaning in this behavior and I should put it back.
- It had a few code paths silently discarding subiterator's status. The stress test caught a few.
- The backwards iteration code path was expecting the internal iterator's set of keys to be immutable. It's probably always true in practice at the moment, since ForwardIterator doesn't support backwards iteration, but this PR fixes it anyway. See added DBIteratorTest.ReverseToForwardBug for an example.
- Some parts of backwards iteration code path even did things like `assert(iter_->Valid())` after a seek, which is never a safe assumption.
- It used to not reset status on seek for some types of errors.
- Some simplifications and better comments.
- Some things got more complicated from the added error handling. I'm open to ideas for how to make it nicer.
* MergingIterator - check status after every operation on every subiterator, and in some places assert that valid subiterators have ok status.
* ForwardIterator - changed to the new convention, also slightly simplified.
* ForwardLevelIterator - fixed some bugs and simplified.
* LevelIterator - simplified.
* TwoLevelIterator - changed to the new convention. Also fixed a bug that would make SeekForPrev() sometimes silently ignore errors from first_level_iter_.
* BlockBasedTableIterator - minor changes.
* BlockIter - replaced `SetStatus()` with `Invalidate()` to make sure non-ok BlockIter is always invalid.
* PlainTableIterator - some seeks used to not reset status.
* CuckooTableIterator - tiny code cleanup.
* ManagedIterator - fixed some bugs.
* BaseDeltaIterator - changed to the new convention and fixed a bug.
* BlobDBIterator - seeks used to not reset status.
* KeyConvertingIterator - some small change.
Closes https://github.com/facebook/rocksdb/pull/3810
Differential Revision: D7888019
Pulled By: al13n321
fbshipit-source-id: 4aaf6d3421c545d16722a815b2fa2e7912bc851d
2018-05-17 09:44:14 +00:00
|
|
|
if (!first_level_iter_.Valid() && first_level_iter_.status().ok()) {
|
2016-09-28 01:20:57 +00:00
|
|
|
first_level_iter_.SeekToLast();
|
|
|
|
InitDataBlock();
|
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.SeekForPrev(target);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
SkipEmptyDataBlocksBackward();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-03-18 22:37:00 +00:00
|
|
|
void TwoLevelIterator::SeekToFirst() {
|
2014-04-25 19:22:23 +00:00
|
|
|
first_level_iter_.SeekToFirst();
|
2011-03-18 22:37:00 +00:00
|
|
|
InitDataBlock();
|
2014-04-25 19:22:23 +00:00
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.SeekToFirst();
|
|
|
|
}
|
2011-03-18 22:37:00 +00:00
|
|
|
SkipEmptyDataBlocksForward();
|
|
|
|
}
|
|
|
|
|
|
|
|
void TwoLevelIterator::SeekToLast() {
|
2014-04-25 19:22:23 +00:00
|
|
|
first_level_iter_.SeekToLast();
|
2011-03-18 22:37:00 +00:00
|
|
|
InitDataBlock();
|
2014-04-25 19:22:23 +00:00
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.SeekToLast();
|
|
|
|
}
|
2011-03-18 22:37:00 +00:00
|
|
|
SkipEmptyDataBlocksBackward();
|
|
|
|
}
|
|
|
|
|
|
|
|
void TwoLevelIterator::Next() {
|
|
|
|
assert(Valid());
|
2014-04-25 19:22:23 +00:00
|
|
|
second_level_iter_.Next();
|
2011-03-18 22:37:00 +00:00
|
|
|
SkipEmptyDataBlocksForward();
|
|
|
|
}
|
|
|
|
|
|
|
|
void TwoLevelIterator::Prev() {
|
|
|
|
assert(Valid());
|
2014-04-25 19:22:23 +00:00
|
|
|
second_level_iter_.Prev();
|
2011-03-18 22:37:00 +00:00
|
|
|
SkipEmptyDataBlocksBackward();
|
|
|
|
}
|
|
|
|
|
|
|
|
void TwoLevelIterator::SkipEmptyDataBlocksForward() {
|
2014-04-25 19:22:23 +00:00
|
|
|
while (second_level_iter_.iter() == nullptr ||
|
Change and clarify the relationship between Valid(), status() and Seek*() for all iterators. Also fix some bugs
Summary:
Before this PR, Iterator/InternalIterator may simultaneously have non-ok status() and Valid() = true. That state means that the last operation failed, but the iterator is nevertheless positioned on some unspecified record. Likely intended uses of that are:
* If some sst files are corrupted, a normal iterator can be used to read the data from files that are not corrupted.
* When using read_tier = kBlockCacheTier, read the data that's in block cache, skipping over the data that is not.
However, this behavior wasn't documented well (and until recently the wiki on github had misleading incorrect information). In the code there's a lot of confusion about the relationship between status() and Valid(), and about whether Seek()/SeekToLast()/etc reset the status or not. There were a number of bugs caused by this confusion, both inside rocksdb and in the code that uses rocksdb (including ours).
This PR changes the convention to:
* If status() is not ok, Valid() always returns false.
* Any seek operation resets status. (Before the PR, it depended on iterator type and on particular error.)
This does sacrifice the two use cases listed above, but siying said it's ok.
Overview of the changes:
* A commit that adds missing status checks in MergingIterator. This fixes a bug that actually affects us, and we need it fixed. `DBIteratorTest.NonBlockingIterationBugRepro` explains the scenario.
* Changes to lots of iterator types to make all of them conform to the new convention. Some bug fixes along the way. By far the biggest changes are in DBIter, which is a big messy piece of code; I tried to make it less big and messy but mostly failed.
* A stress-test for DBIter, to gain some confidence that I didn't break it. It does a few million random operations on the iterator, while occasionally modifying the underlying data (like ForwardIterator does) and occasionally returning non-ok status from internal iterator.
To find the iterator types that needed changes I searched for "public .*Iterator" in the code. Here's an overview of all 27 iterator types:
Iterators that didn't need changes:
* status() is always ok(), or Valid() is always false: MemTableIterator, ModelIter, TestIterator, KVIter (2 classes with this name anonymous namespaces), LoggingForwardVectorIterator, VectorIterator, MockTableIterator, EmptyIterator, EmptyInternalIterator.
* Thin wrappers that always pass through Valid() and status(): ArenaWrappedDBIter, TtlIterator, InternalIteratorFromIterator.
Iterators with changes (see inline comments for details):
* DBIter - an overhaul:
- It used to silently skip corrupted keys (`FindParseableKey()`), which seems dangerous. This PR makes it just stop immediately after encountering a corrupted key, just like it would for other kinds of corruption. Let me know if there was actually some deeper meaning in this behavior and I should put it back.
- It had a few code paths silently discarding subiterator's status. The stress test caught a few.
- The backwards iteration code path was expecting the internal iterator's set of keys to be immutable. It's probably always true in practice at the moment, since ForwardIterator doesn't support backwards iteration, but this PR fixes it anyway. See added DBIteratorTest.ReverseToForwardBug for an example.
- Some parts of backwards iteration code path even did things like `assert(iter_->Valid())` after a seek, which is never a safe assumption.
- It used to not reset status on seek for some types of errors.
- Some simplifications and better comments.
- Some things got more complicated from the added error handling. I'm open to ideas for how to make it nicer.
* MergingIterator - check status after every operation on every subiterator, and in some places assert that valid subiterators have ok status.
* ForwardIterator - changed to the new convention, also slightly simplified.
* ForwardLevelIterator - fixed some bugs and simplified.
* LevelIterator - simplified.
* TwoLevelIterator - changed to the new convention. Also fixed a bug that would make SeekForPrev() sometimes silently ignore errors from first_level_iter_.
* BlockBasedTableIterator - minor changes.
* BlockIter - replaced `SetStatus()` with `Invalidate()` to make sure non-ok BlockIter is always invalid.
* PlainTableIterator - some seeks used to not reset status.
* CuckooTableIterator - tiny code cleanup.
* ManagedIterator - fixed some bugs.
* BaseDeltaIterator - changed to the new convention and fixed a bug.
* BlobDBIterator - seeks used to not reset status.
* KeyConvertingIterator - some small change.
Closes https://github.com/facebook/rocksdb/pull/3810
Differential Revision: D7888019
Pulled By: al13n321
fbshipit-source-id: 4aaf6d3421c545d16722a815b2fa2e7912bc851d
2018-05-17 09:44:14 +00:00
|
|
|
(!second_level_iter_.Valid() && second_level_iter_.status().ok())) {
|
2011-03-18 22:37:00 +00:00
|
|
|
// Move to next block
|
2018-02-13 00:57:56 +00:00
|
|
|
if (!first_level_iter_.Valid()) {
|
2014-04-25 19:22:23 +00:00
|
|
|
SetSecondLevelIterator(nullptr);
|
2011-03-18 22:37:00 +00:00
|
|
|
return;
|
|
|
|
}
|
2014-04-25 19:22:23 +00:00
|
|
|
first_level_iter_.Next();
|
2011-03-18 22:37:00 +00:00
|
|
|
InitDataBlock();
|
2014-04-25 19:22:23 +00:00
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.SeekToFirst();
|
|
|
|
}
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
void TwoLevelIterator::SkipEmptyDataBlocksBackward() {
|
2014-04-25 19:22:23 +00:00
|
|
|
while (second_level_iter_.iter() == nullptr ||
|
Change and clarify the relationship between Valid(), status() and Seek*() for all iterators. Also fix some bugs
Summary:
Before this PR, Iterator/InternalIterator may simultaneously have non-ok status() and Valid() = true. That state means that the last operation failed, but the iterator is nevertheless positioned on some unspecified record. Likely intended uses of that are:
* If some sst files are corrupted, a normal iterator can be used to read the data from files that are not corrupted.
* When using read_tier = kBlockCacheTier, read the data that's in block cache, skipping over the data that is not.
However, this behavior wasn't documented well (and until recently the wiki on github had misleading incorrect information). In the code there's a lot of confusion about the relationship between status() and Valid(), and about whether Seek()/SeekToLast()/etc reset the status or not. There were a number of bugs caused by this confusion, both inside rocksdb and in the code that uses rocksdb (including ours).
This PR changes the convention to:
* If status() is not ok, Valid() always returns false.
* Any seek operation resets status. (Before the PR, it depended on iterator type and on particular error.)
This does sacrifice the two use cases listed above, but siying said it's ok.
Overview of the changes:
* A commit that adds missing status checks in MergingIterator. This fixes a bug that actually affects us, and we need it fixed. `DBIteratorTest.NonBlockingIterationBugRepro` explains the scenario.
* Changes to lots of iterator types to make all of them conform to the new convention. Some bug fixes along the way. By far the biggest changes are in DBIter, which is a big messy piece of code; I tried to make it less big and messy but mostly failed.
* A stress-test for DBIter, to gain some confidence that I didn't break it. It does a few million random operations on the iterator, while occasionally modifying the underlying data (like ForwardIterator does) and occasionally returning non-ok status from internal iterator.
To find the iterator types that needed changes I searched for "public .*Iterator" in the code. Here's an overview of all 27 iterator types:
Iterators that didn't need changes:
* status() is always ok(), or Valid() is always false: MemTableIterator, ModelIter, TestIterator, KVIter (2 classes with this name anonymous namespaces), LoggingForwardVectorIterator, VectorIterator, MockTableIterator, EmptyIterator, EmptyInternalIterator.
* Thin wrappers that always pass through Valid() and status(): ArenaWrappedDBIter, TtlIterator, InternalIteratorFromIterator.
Iterators with changes (see inline comments for details):
* DBIter - an overhaul:
- It used to silently skip corrupted keys (`FindParseableKey()`), which seems dangerous. This PR makes it just stop immediately after encountering a corrupted key, just like it would for other kinds of corruption. Let me know if there was actually some deeper meaning in this behavior and I should put it back.
- It had a few code paths silently discarding subiterator's status. The stress test caught a few.
- The backwards iteration code path was expecting the internal iterator's set of keys to be immutable. It's probably always true in practice at the moment, since ForwardIterator doesn't support backwards iteration, but this PR fixes it anyway. See added DBIteratorTest.ReverseToForwardBug for an example.
- Some parts of backwards iteration code path even did things like `assert(iter_->Valid())` after a seek, which is never a safe assumption.
- It used to not reset status on seek for some types of errors.
- Some simplifications and better comments.
- Some things got more complicated from the added error handling. I'm open to ideas for how to make it nicer.
* MergingIterator - check status after every operation on every subiterator, and in some places assert that valid subiterators have ok status.
* ForwardIterator - changed to the new convention, also slightly simplified.
* ForwardLevelIterator - fixed some bugs and simplified.
* LevelIterator - simplified.
* TwoLevelIterator - changed to the new convention. Also fixed a bug that would make SeekForPrev() sometimes silently ignore errors from first_level_iter_.
* BlockBasedTableIterator - minor changes.
* BlockIter - replaced `SetStatus()` with `Invalidate()` to make sure non-ok BlockIter is always invalid.
* PlainTableIterator - some seeks used to not reset status.
* CuckooTableIterator - tiny code cleanup.
* ManagedIterator - fixed some bugs.
* BaseDeltaIterator - changed to the new convention and fixed a bug.
* BlobDBIterator - seeks used to not reset status.
* KeyConvertingIterator - some small change.
Closes https://github.com/facebook/rocksdb/pull/3810
Differential Revision: D7888019
Pulled By: al13n321
fbshipit-source-id: 4aaf6d3421c545d16722a815b2fa2e7912bc851d
2018-05-17 09:44:14 +00:00
|
|
|
(!second_level_iter_.Valid() && second_level_iter_.status().ok())) {
|
2011-03-18 22:37:00 +00:00
|
|
|
// Move to next block
|
2014-04-25 19:22:23 +00:00
|
|
|
if (!first_level_iter_.Valid()) {
|
|
|
|
SetSecondLevelIterator(nullptr);
|
2011-03-18 22:37:00 +00:00
|
|
|
return;
|
|
|
|
}
|
2014-04-25 19:22:23 +00:00
|
|
|
first_level_iter_.Prev();
|
2011-03-18 22:37:00 +00:00
|
|
|
InitDataBlock();
|
2014-04-25 19:22:23 +00:00
|
|
|
if (second_level_iter_.iter() != nullptr) {
|
|
|
|
second_level_iter_.SeekToLast();
|
|
|
|
}
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2015-10-12 22:06:38 +00:00
|
|
|
void TwoLevelIterator::SetSecondLevelIterator(InternalIterator* iter) {
|
2016-04-26 19:41:07 +00:00
|
|
|
InternalIterator* old_iter = second_level_iter_.Set(iter);
|
2018-02-13 00:57:56 +00:00
|
|
|
delete old_iter;
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
void TwoLevelIterator::InitDataBlock() {
|
2014-04-25 19:22:23 +00:00
|
|
|
if (!first_level_iter_.Valid()) {
|
|
|
|
SetSecondLevelIterator(nullptr);
|
2011-03-18 22:37:00 +00:00
|
|
|
} else {
|
2014-04-25 19:22:23 +00:00
|
|
|
Slice handle = first_level_iter_.value();
|
2014-08-29 21:32:37 +00:00
|
|
|
if (second_level_iter_.iter() != nullptr &&
|
|
|
|
!second_level_iter_.status().IsIncomplete() &&
|
|
|
|
handle.compare(data_block_handle_) == 0) {
|
2014-04-25 19:22:23 +00:00
|
|
|
// second_level_iter is already constructed with this iterator, so
|
2011-03-18 22:37:00 +00:00
|
|
|
// no need to change anything
|
|
|
|
} else {
|
2015-10-12 22:06:38 +00:00
|
|
|
InternalIterator* iter = state_->NewSecondaryIterator(handle);
|
2011-03-18 22:37:00 +00:00
|
|
|
data_block_handle_.assign(handle.data(), handle.size());
|
2014-04-25 19:22:23 +00:00
|
|
|
SetSecondLevelIterator(iter);
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2011-10-31 17:22:06 +00:00
|
|
|
} // namespace
|
2011-03-18 22:37:00 +00:00
|
|
|
|
2015-10-12 22:06:38 +00:00
|
|
|
InternalIterator* NewTwoLevelIterator(TwoLevelIteratorState* state,
|
2018-02-13 00:57:56 +00:00
|
|
|
InternalIterator* first_level_iter) {
|
|
|
|
return new TwoLevelIterator(state, first_level_iter);
|
2011-03-18 22:37:00 +00:00
|
|
|
}
|
2013-10-04 04:49:15 +00:00
|
|
|
} // namespace rocksdb
|