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).
|
2014-05-30 21:31:55 +00:00
|
|
|
|
|
|
|
#include "db/forward_iterator.h"
|
|
|
|
|
2014-08-29 21:32:37 +00:00
|
|
|
#include <limits>
|
2014-05-30 21:31:55 +00:00
|
|
|
#include <string>
|
|
|
|
#include <utility>
|
2014-08-29 21:32:37 +00:00
|
|
|
|
2016-03-01 02:38:03 +00:00
|
|
|
#include "db/column_family.h"
|
2019-05-31 18:52:59 +00:00
|
|
|
#include "db/db_impl/db_impl.h"
|
2014-05-30 21:31:55 +00:00
|
|
|
#include "db/db_iter.h"
|
2016-03-01 02:38:03 +00:00
|
|
|
#include "db/dbformat.h"
|
|
|
|
#include "db/job_context.h"
|
2018-12-18 01:26:56 +00:00
|
|
|
#include "db/range_del_aggregator.h"
|
2018-11-28 23:26:56 +00:00
|
|
|
#include "db/range_tombstone_fragmenter.h"
|
2014-05-30 21:31:55 +00:00
|
|
|
#include "rocksdb/env.h"
|
|
|
|
#include "rocksdb/slice.h"
|
|
|
|
#include "rocksdb/slice_transform.h"
|
2017-02-03 00:38:40 +00:00
|
|
|
#include "table/merging_iterator.h"
|
2019-05-30 18:21:38 +00:00
|
|
|
#include "test_util/sync_point.h"
|
2019-05-31 00:39:43 +00:00
|
|
|
#include "util/string_util.h"
|
2014-05-30 21:31:55 +00:00
|
|
|
|
2020-02-20 20:07:53 +00:00
|
|
|
namespace ROCKSDB_NAMESPACE {
|
2014-05-30 21:31:55 +00:00
|
|
|
|
|
|
|
// Usage:
|
2018-02-13 21:44:22 +00:00
|
|
|
// ForwardLevelIterator iter;
|
2014-05-30 21:31:55 +00:00
|
|
|
// iter.SetFileIndex(file_index);
|
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
|
|
|
// iter.Seek(target); // or iter.SeekToFirst();
|
2014-05-30 21:31:55 +00:00
|
|
|
// iter.Next()
|
2018-02-13 21:44:22 +00:00
|
|
|
class ForwardLevelIterator : public InternalIterator {
|
2014-05-30 21:31:55 +00:00
|
|
|
public:
|
2022-01-21 19:36:36 +00:00
|
|
|
ForwardLevelIterator(
|
|
|
|
const ColumnFamilyData* const cfd, const ReadOptions& read_options,
|
|
|
|
const std::vector<FileMetaData*>& files,
|
|
|
|
const std::shared_ptr<const SliceTransform>& prefix_extractor,
|
2023-04-25 19:08:23 +00:00
|
|
|
bool allow_unprepared_value, uint8_t block_protection_bytes_per_key)
|
2016-08-12 02:10:16 +00:00
|
|
|
: cfd_(cfd),
|
|
|
|
read_options_(read_options),
|
|
|
|
files_(files),
|
|
|
|
valid_(false),
|
|
|
|
file_index_(std::numeric_limits<uint32_t>::max()),
|
|
|
|
file_iter_(nullptr),
|
2018-05-21 21:33:55 +00:00
|
|
|
pinned_iters_mgr_(nullptr),
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
prefix_extractor_(prefix_extractor),
|
2023-04-25 19:08:23 +00:00
|
|
|
allow_unprepared_value_(allow_unprepared_value),
|
|
|
|
block_protection_bytes_per_key_(block_protection_bytes_per_key) {
|
2020-12-23 07:44:44 +00:00
|
|
|
status_.PermitUncheckedError(); // Allow uninitialized status through
|
2020-12-08 23:53:59 +00:00
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
|
2019-02-14 21:52:47 +00:00
|
|
|
~ForwardLevelIterator() override {
|
2016-08-12 06:34:19 +00:00
|
|
|
// Reset current pointer
|
|
|
|
if (pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled()) {
|
|
|
|
pinned_iters_mgr_->PinIterator(file_iter_);
|
|
|
|
} else {
|
|
|
|
delete file_iter_;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
void SetFileIndex(uint32_t file_index) {
|
|
|
|
assert(file_index < files_.size());
|
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
|
|
|
status_ = Status::OK();
|
2014-05-30 21:31:55 +00:00
|
|
|
if (file_index != file_index_) {
|
|
|
|
file_index_ = file_index;
|
2014-08-29 21:32:37 +00:00
|
|
|
Reset();
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
}
|
2014-08-29 21:32:37 +00:00
|
|
|
void Reset() {
|
|
|
|
assert(file_index_ < files_.size());
|
2016-08-12 02:10:16 +00:00
|
|
|
|
|
|
|
// Reset current pointer
|
|
|
|
if (pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled()) {
|
|
|
|
pinned_iters_mgr_->PinIterator(file_iter_);
|
|
|
|
} else {
|
|
|
|
delete file_iter_;
|
|
|
|
}
|
|
|
|
|
2018-12-18 01:26:56 +00:00
|
|
|
ReadRangeDelAggregator range_del_agg(&cfd_->internal_comparator(),
|
|
|
|
kMaxSequenceNumber /* upper_bound */);
|
2016-08-12 02:10:16 +00:00
|
|
|
file_iter_ = cfd_->table_cache()->NewIterator(
|
2014-08-29 21:32:37 +00:00
|
|
|
read_options_, *(cfd_->soptions()), cfd_->internal_comparator(),
|
2018-07-14 00:34:54 +00:00
|
|
|
*files_[file_index_],
|
2017-05-05 22:01:04 +00:00
|
|
|
read_options_.ignore_range_deletions ? nullptr : &range_del_agg,
|
2019-06-20 21:28:22 +00:00
|
|
|
prefix_extractor_, /*table_reader_ptr=*/nullptr,
|
|
|
|
/*file_read_hist=*/nullptr, TableReaderCaller::kUserIterator,
|
|
|
|
/*arena=*/nullptr, /*skip_filters=*/false, /*level=*/-1,
|
2020-06-09 23:49:07 +00:00
|
|
|
/*max_file_size_for_l0_meta_pin=*/0,
|
2019-06-20 21:28:22 +00:00
|
|
|
/*smallest_compaction_key=*/nullptr,
|
2023-04-25 19:08:23 +00:00
|
|
|
/*largest_compaction_key=*/nullptr, allow_unprepared_value_,
|
|
|
|
block_protection_bytes_per_key_);
|
2016-08-12 02:10:16 +00:00
|
|
|
file_iter_->SetPinnedItersMgr(pinned_iters_mgr_);
|
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
|
|
|
valid_ = false;
|
2017-01-23 21:30:14 +00:00
|
|
|
if (!range_del_agg.IsEmpty()) {
|
|
|
|
status_ = Status::NotSupported(
|
|
|
|
"Range tombstones unsupported with ForwardIterator");
|
|
|
|
}
|
2014-08-29 21:32:37 +00:00
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
void SeekToLast() override {
|
2018-02-13 21:44:22 +00:00
|
|
|
status_ = Status::NotSupported("ForwardLevelIterator::SeekToLast()");
|
2014-05-30 21:31:55 +00:00
|
|
|
valid_ = false;
|
|
|
|
}
|
2015-02-26 19:28:41 +00:00
|
|
|
void Prev() override {
|
2018-02-13 21:44:22 +00:00
|
|
|
status_ = Status::NotSupported("ForwardLevelIterator::Prev()");
|
2014-05-30 21:31:55 +00:00
|
|
|
valid_ = false;
|
|
|
|
}
|
2022-11-02 21:34:24 +00:00
|
|
|
bool Valid() const override { return valid_; }
|
2014-05-30 21:31:55 +00:00
|
|
|
void SeekToFirst() override {
|
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(file_iter_ != nullptr);
|
|
|
|
if (!status_.ok()) {
|
|
|
|
assert(!valid_);
|
|
|
|
return;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
file_iter_->SeekToFirst();
|
|
|
|
valid_ = file_iter_->Valid();
|
|
|
|
}
|
|
|
|
void Seek(const Slice& internal_key) override {
|
|
|
|
assert(file_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
|
|
|
|
|
|
|
// This deviates from the usual convention for InternalIterator::Seek() in
|
|
|
|
// that it doesn't discard pre-existing error status. That's because this
|
|
|
|
// Seek() is only supposed to be called immediately after SetFileIndex()
|
|
|
|
// (which discards pre-existing error status), and SetFileIndex() may set
|
|
|
|
// an error status, which we shouldn't discard.
|
|
|
|
if (!status_.ok()) {
|
|
|
|
assert(!valid_);
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
file_iter_->Seek(internal_key);
|
|
|
|
valid_ = file_iter_->Valid();
|
|
|
|
}
|
2018-03-05 21:08:17 +00:00
|
|
|
void SeekForPrev(const Slice& /*internal_key*/) override {
|
2018-02-13 21:44:22 +00:00
|
|
|
status_ = Status::NotSupported("ForwardLevelIterator::SeekForPrev()");
|
2016-09-28 01:20:57 +00:00
|
|
|
valid_ = false;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
void Next() override {
|
|
|
|
assert(valid_);
|
|
|
|
file_iter_->Next();
|
2014-08-29 21:32:37 +00:00
|
|
|
for (;;) {
|
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
|
|
|
valid_ = file_iter_->Valid();
|
|
|
|
if (!file_iter_->status().ok()) {
|
|
|
|
assert(!valid_);
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
if (valid_) {
|
2014-08-29 21:32:37 +00:00
|
|
|
return;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
if (file_index_ + 1 >= files_.size()) {
|
|
|
|
valid_ = false;
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
SetFileIndex(file_index_ + 1);
|
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 (!status_.ok()) {
|
|
|
|
assert(!valid_);
|
|
|
|
return;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
file_iter_->SeekToFirst();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
Slice key() const override {
|
|
|
|
assert(valid_);
|
|
|
|
return file_iter_->key();
|
|
|
|
}
|
|
|
|
Slice value() const override {
|
|
|
|
assert(valid_);
|
|
|
|
return file_iter_->value();
|
|
|
|
}
|
|
|
|
Status status() const override {
|
2014-07-10 00:46:18 +00:00
|
|
|
if (!status_.ok()) {
|
|
|
|
return status_;
|
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
|
|
|
} else if (file_iter_) {
|
2014-07-10 00:46:18 +00:00
|
|
|
return file_iter_->status();
|
|
|
|
}
|
|
|
|
return Status::OK();
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
bool PrepareValue() override {
|
|
|
|
assert(valid_);
|
|
|
|
if (file_iter_->PrepareValue()) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
|
|
|
assert(!file_iter_->Valid());
|
|
|
|
valid_ = false;
|
|
|
|
return false;
|
|
|
|
}
|
2016-08-12 02:10:16 +00:00
|
|
|
bool IsKeyPinned() const override {
|
|
|
|
return pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled() &&
|
|
|
|
file_iter_->IsKeyPinned();
|
|
|
|
}
|
|
|
|
bool IsValuePinned() const override {
|
|
|
|
return pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled() &&
|
|
|
|
file_iter_->IsValuePinned();
|
|
|
|
}
|
|
|
|
void SetPinnedItersMgr(PinnedIteratorsManager* pinned_iters_mgr) override {
|
|
|
|
pinned_iters_mgr_ = pinned_iters_mgr;
|
|
|
|
if (file_iter_) {
|
|
|
|
file_iter_->SetPinnedItersMgr(pinned_iters_mgr_);
|
|
|
|
}
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
|
|
|
|
private:
|
|
|
|
const ColumnFamilyData* const cfd_;
|
|
|
|
const ReadOptions& read_options_;
|
|
|
|
const std::vector<FileMetaData*>& files_;
|
|
|
|
|
|
|
|
bool valid_;
|
|
|
|
uint32_t file_index_;
|
|
|
|
Status status_;
|
2016-08-12 02:10:16 +00:00
|
|
|
InternalIterator* file_iter_;
|
|
|
|
PinnedIteratorsManager* pinned_iters_mgr_;
|
2022-01-21 19:36:36 +00:00
|
|
|
// Kept alive by ForwardIterator::sv_->mutable_cf_options
|
|
|
|
const std::shared_ptr<const SliceTransform>& prefix_extractor_;
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
const bool allow_unprepared_value_;
|
2023-04-25 19:08:23 +00:00
|
|
|
const uint8_t block_protection_bytes_per_key_;
|
2014-05-30 21:31:55 +00:00
|
|
|
};
|
|
|
|
|
2014-06-03 19:28:58 +00:00
|
|
|
ForwardIterator::ForwardIterator(DBImpl* db, const ReadOptions& read_options,
|
2015-08-19 23:05:51 +00:00
|
|
|
ColumnFamilyData* cfd,
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
SuperVersion* current_sv,
|
|
|
|
bool allow_unprepared_value)
|
2014-05-30 21:31:55 +00:00
|
|
|
: db_(db),
|
|
|
|
read_options_(read_options),
|
|
|
|
cfd_(cfd),
|
2018-05-21 21:33:55 +00:00
|
|
|
prefix_extractor_(current_sv->mutable_cf_options.prefix_extractor.get()),
|
2014-05-30 21:31:55 +00:00
|
|
|
user_comparator_(cfd->user_comparator()),
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
allow_unprepared_value_(allow_unprepared_value),
|
2014-05-30 21:31:55 +00:00
|
|
|
immutable_min_heap_(MinIterComparator(&cfd_->internal_comparator())),
|
2014-10-23 22:34:21 +00:00
|
|
|
sv_(current_sv),
|
2014-05-30 21:31:55 +00:00
|
|
|
mutable_iter_(nullptr),
|
|
|
|
current_(nullptr),
|
2015-08-31 23:44:34 +00:00
|
|
|
valid_(false),
|
2014-09-26 21:20:24 +00:00
|
|
|
status_(Status::OK()),
|
|
|
|
immutable_status_(Status::OK()),
|
2015-08-19 23:05:51 +00:00
|
|
|
has_iter_trimmed_for_upper_bound_(false),
|
2015-08-31 23:44:34 +00:00
|
|
|
current_over_upper_bound_(false),
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
is_prev_set_(false),
|
2016-08-12 02:10:16 +00:00
|
|
|
is_prev_inclusive_(false),
|
|
|
|
pinned_iters_mgr_(nullptr) {
|
2014-10-23 22:34:21 +00:00
|
|
|
if (sv_) {
|
|
|
|
RebuildIterators(false);
|
|
|
|
}
|
2023-06-23 18:48:49 +00:00
|
|
|
if (!CheckFSFeatureSupport(cfd_->ioptions()->env->GetFileSystem().get(),
|
|
|
|
FSSupportedOps::kAsyncIO)) {
|
2023-03-17 21:57:09 +00:00
|
|
|
read_options_.async_io = false;
|
|
|
|
}
|
2020-12-22 23:08:17 +00:00
|
|
|
// immutable_status_ is a local aggregation of the
|
|
|
|
// status of the immutable Iterators.
|
|
|
|
// We have to PermitUncheckedError in case it is never
|
|
|
|
// used, otherwise it will fail ASSERT_STATUS_CHECKED.
|
|
|
|
immutable_status_.PermitUncheckedError();
|
2014-10-23 22:34:21 +00:00
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
|
2022-11-02 21:34:24 +00:00
|
|
|
ForwardIterator::~ForwardIterator() { Cleanup(true); }
|
2014-05-30 21:31:55 +00:00
|
|
|
|
2018-02-03 05:16:26 +00:00
|
|
|
void ForwardIterator::SVCleanup(DBImpl* db, SuperVersion* sv,
|
|
|
|
bool background_purge_on_iterator_cleanup) {
|
|
|
|
if (sv->Unref()) {
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
// Job id == 0 means that this is not our background process, but rather
|
|
|
|
// user thread
|
|
|
|
JobContext job_context(0);
|
2018-02-03 05:16:26 +00:00
|
|
|
db->mutex_.Lock();
|
|
|
|
sv->Cleanup();
|
|
|
|
db->FindObsoleteFiles(&job_context, false, true);
|
|
|
|
if (background_purge_on_iterator_cleanup) {
|
|
|
|
db->ScheduleBgLogWriterClose(&job_context);
|
2019-12-17 21:20:42 +00:00
|
|
|
db->AddSuperVersionsToFreeQueue(sv);
|
|
|
|
db->SchedulePurge();
|
2016-08-12 02:10:16 +00:00
|
|
|
}
|
2018-02-03 05:16:26 +00:00
|
|
|
db->mutex_.Unlock();
|
2019-12-17 21:20:42 +00:00
|
|
|
if (!background_purge_on_iterator_cleanup) {
|
|
|
|
delete sv;
|
|
|
|
}
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
if (job_context.HaveSomethingToDelete()) {
|
2018-02-03 05:16:26 +00:00
|
|
|
db->PurgeObsoleteFiles(job_context, background_purge_on_iterator_cleanup);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
job_context.Clean();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2018-02-03 05:16:26 +00:00
|
|
|
namespace {
|
|
|
|
struct SVCleanupParams {
|
|
|
|
DBImpl* db;
|
|
|
|
SuperVersion* sv;
|
|
|
|
bool background_purge_on_iterator_cleanup;
|
|
|
|
};
|
2022-11-02 21:34:24 +00:00
|
|
|
} // anonymous namespace
|
2018-02-03 05:16:26 +00:00
|
|
|
|
|
|
|
// Used in PinnedIteratorsManager to release pinned SuperVersion
|
|
|
|
void ForwardIterator::DeferredSVCleanup(void* arg) {
|
Prefer static_cast in place of most reinterpret_cast (#12308)
Summary:
The following are risks associated with pointer-to-pointer reinterpret_cast:
* Can produce the "wrong result" (crash or memory corruption). IIRC, in theory this can happen for any up-cast or down-cast for a non-standard-layout type, though in practice would only happen for multiple inheritance cases (where the base class pointer might be "inside" the derived object). We don't use multiple inheritance a lot, but we do.
* Can mask useful compiler errors upon code change, including converting between unrelated pointer types that you are expecting to be related, and converting between pointer and scalar types unintentionally.
I can only think of some obscure cases where static_cast could be troublesome when it compiles as a replacement:
* Going through `void*` could plausibly cause unnecessary or broken pointer arithmetic. Suppose we have
`struct Derived: public Base1, public Base2`. If we have `Derived*` -> `void*` -> `Base2*` -> `Derived*` through reinterpret casts, this could plausibly work (though technical UB) assuming the `Base2*` is not dereferenced. Changing to static cast could introduce breaking pointer arithmetic.
* Unnecessary (but safe) pointer arithmetic could arise in a case like `Derived*` -> `Base2*` -> `Derived*` where before the Base2 pointer might not have been dereferenced. This could potentially affect performance.
With some light scripting, I tried replacing pointer-to-pointer reinterpret_casts with static_cast and kept the cases that still compile. Most occurrences of reinterpret_cast have successfully been changed (except for java/ and third-party/). 294 changed, 257 remain.
A couple of related interventions included here:
* Previously Cache::Handle was not actually derived from in the implementations and just used as a `void*` stand-in with reinterpret_cast. Now there is a relationship to allow static_cast. In theory, this could introduce pointer arithmetic (as described above) but is unlikely without multiple inheritance AND non-empty Cache::Handle.
* Remove some unnecessary casts to void* as this is allowed to be implicit (for better or worse).
Most of the remaining reinterpret_casts are for converting to/from raw bytes of objects. We could consider better idioms for these patterns in follow-up work.
I wish there were a way to implement a template variant of static_cast that would only compile if no pointer arithmetic is generated, but best I can tell, this is not possible. AFAIK the best you could do is a dynamic check that the void* conversion after the static cast is unchanged.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12308
Test Plan: existing tests, CI
Reviewed By: ltamasi
Differential Revision: D53204947
Pulled By: pdillinger
fbshipit-source-id: 9de23e618263b0d5b9820f4e15966876888a16e2
2024-02-07 18:44:11 +00:00
|
|
|
auto d = static_cast<SVCleanupParams*>(arg);
|
2022-11-02 21:34:24 +00:00
|
|
|
ForwardIterator::SVCleanup(d->db, d->sv,
|
|
|
|
d->background_purge_on_iterator_cleanup);
|
2018-02-03 05:16:26 +00:00
|
|
|
delete d;
|
|
|
|
}
|
|
|
|
|
|
|
|
void ForwardIterator::SVCleanup() {
|
|
|
|
if (sv_ == nullptr) {
|
|
|
|
return;
|
|
|
|
}
|
2019-04-02 00:07:38 +00:00
|
|
|
bool background_purge =
|
|
|
|
read_options_.background_purge_on_iterator_cleanup ||
|
|
|
|
db_->immutable_db_options().avoid_unnecessary_blocking_io;
|
2018-02-03 05:16:26 +00:00
|
|
|
if (pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled()) {
|
|
|
|
// pinned_iters_mgr_ tells us to make sure that all visited key-value slices
|
|
|
|
// are alive until pinned_iters_mgr_->ReleasePinnedData() is called.
|
|
|
|
// The slices may point into some memtables owned by sv_, so we need to keep
|
|
|
|
// sv_ referenced until pinned_iters_mgr_ unpins everything.
|
2019-04-02 00:07:38 +00:00
|
|
|
auto p = new SVCleanupParams{db_, sv_, background_purge};
|
2018-02-03 05:16:26 +00:00
|
|
|
pinned_iters_mgr_->PinPtr(p, &ForwardIterator::DeferredSVCleanup);
|
|
|
|
} else {
|
2019-04-02 00:07:38 +00:00
|
|
|
SVCleanup(db_, sv_, background_purge);
|
2018-02-03 05:16:26 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2014-10-23 22:34:21 +00:00
|
|
|
void ForwardIterator::Cleanup(bool release_sv) {
|
2014-09-05 00:40:41 +00:00
|
|
|
if (mutable_iter_ != nullptr) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(mutable_iter_, true /* is_arena */);
|
2014-09-05 00:40:41 +00:00
|
|
|
}
|
2016-08-12 02:10:16 +00:00
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
for (auto* m : imm_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(m, true /* is_arena */);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
imm_iters_.clear();
|
2016-08-12 02:10:16 +00:00
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
for (auto* f : l0_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(f);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
l0_iters_.clear();
|
2016-08-12 02:10:16 +00:00
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
for (auto* l : level_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(l);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
level_iters_.clear();
|
|
|
|
|
2014-10-23 22:34:21 +00:00
|
|
|
if (release_sv) {
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
SVCleanup();
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
bool ForwardIterator::Valid() const {
|
2015-08-31 23:44:34 +00:00
|
|
|
// See UpdateCurrent().
|
|
|
|
return valid_ ? !current_over_upper_bound_ : false;
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
void ForwardIterator::SeekToFirst() {
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
if (sv_ == nullptr) {
|
2014-10-23 22:34:21 +00:00
|
|
|
RebuildIterators(true);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
} else if (sv_->version_number != cfd_->GetSuperVersionNumber()) {
|
|
|
|
RenewIterators();
|
2014-09-26 21:20:24 +00:00
|
|
|
} else if (immutable_status_.IsIncomplete()) {
|
2014-08-29 21:32:37 +00:00
|
|
|
ResetIncompleteIterators();
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
2022-10-10 22:48:48 +00:00
|
|
|
SeekInternal(Slice(), true, false);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
2015-08-19 23:05:51 +00:00
|
|
|
bool ForwardIterator::IsOverUpperBound(const Slice& internal_key) const {
|
|
|
|
return !(read_options_.iterate_upper_bound == nullptr ||
|
|
|
|
cfd_->internal_comparator().user_comparator()->Compare(
|
|
|
|
ExtractUserKey(internal_key),
|
|
|
|
*read_options_.iterate_upper_bound) < 0);
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
void ForwardIterator::Seek(const Slice& internal_key) {
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
if (sv_ == nullptr) {
|
2014-10-23 22:34:21 +00:00
|
|
|
RebuildIterators(true);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
} else if (sv_->version_number != cfd_->GetSuperVersionNumber()) {
|
|
|
|
RenewIterators();
|
2014-09-26 21:20:24 +00:00
|
|
|
} else if (immutable_status_.IsIncomplete()) {
|
2014-08-29 21:32:37 +00:00
|
|
|
ResetIncompleteIterators();
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
2022-10-10 22:48:48 +00:00
|
|
|
|
|
|
|
SeekInternal(internal_key, false, false);
|
|
|
|
if (read_options_.async_io) {
|
|
|
|
SeekInternal(internal_key, false, true);
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
2022-10-10 22:48:48 +00:00
|
|
|
// In case of async_io, SeekInternal is called twice with seek_after_async_io
|
|
|
|
// enabled in second call which only does seeking part to retrieve the blocks.
|
2014-05-30 21:31:55 +00:00
|
|
|
void ForwardIterator::SeekInternal(const Slice& internal_key,
|
2022-10-10 22:48:48 +00:00
|
|
|
bool seek_to_first,
|
|
|
|
bool seek_after_async_io) {
|
2014-10-23 22:34:21 +00:00
|
|
|
assert(mutable_iter_);
|
2014-05-30 21:31:55 +00:00
|
|
|
// mutable
|
2022-10-10 22:48:48 +00:00
|
|
|
if (!seek_after_async_io) {
|
|
|
|
seek_to_first ? mutable_iter_->SeekToFirst()
|
|
|
|
: mutable_iter_->Seek(internal_key);
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
|
|
|
|
// immutable
|
|
|
|
// TODO(ljin): NeedToSeekImmutable has negative impact on performance
|
|
|
|
// if it turns to need to seek immutable often. We probably want to have
|
|
|
|
// an option to turn it off.
|
2022-10-10 22:48:48 +00:00
|
|
|
if (seek_to_first || seek_after_async_io ||
|
|
|
|
NeedToSeekImmutable(internal_key)) {
|
|
|
|
if (!seek_after_async_io) {
|
|
|
|
immutable_status_ = Status::OK();
|
|
|
|
if (has_iter_trimmed_for_upper_bound_ &&
|
|
|
|
(
|
|
|
|
// prev_ is not set yet
|
|
|
|
is_prev_set_ == false ||
|
|
|
|
// We are doing SeekToFirst() and internal_key.size() = 0
|
|
|
|
seek_to_first ||
|
|
|
|
// prev_key_ > internal_key
|
|
|
|
cfd_->internal_comparator().InternalKeyComparator::Compare(
|
|
|
|
prev_key_.GetInternalKey(), internal_key) > 0)) {
|
|
|
|
// Some iterators are trimmed. Need to rebuild.
|
|
|
|
RebuildIterators(true);
|
|
|
|
// Already seeked mutable iter, so seek again
|
|
|
|
seek_to_first ? mutable_iter_->SeekToFirst()
|
|
|
|
: mutable_iter_->Seek(internal_key);
|
|
|
|
}
|
|
|
|
{
|
|
|
|
auto tmp = MinIterHeap(MinIterComparator(&cfd_->internal_comparator()));
|
|
|
|
immutable_min_heap_.swap(tmp);
|
|
|
|
}
|
|
|
|
for (size_t i = 0; i < imm_iters_.size(); i++) {
|
|
|
|
auto* m = imm_iters_[i];
|
|
|
|
seek_to_first ? m->SeekToFirst() : m->Seek(internal_key);
|
|
|
|
if (!m->status().ok()) {
|
|
|
|
immutable_status_ = m->status();
|
|
|
|
} else if (m->Valid()) {
|
|
|
|
immutable_min_heap_.push(m);
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2019-04-16 18:32:03 +00:00
|
|
|
Slice target_user_key;
|
2014-07-08 18:40:42 +00:00
|
|
|
if (!seek_to_first) {
|
2019-04-16 18:32:03 +00:00
|
|
|
target_user_key = ExtractUserKey(internal_key);
|
2014-07-08 18:40:42 +00:00
|
|
|
}
|
2014-10-31 15:48:19 +00:00
|
|
|
const VersionStorageInfo* vstorage = sv_->current->storage_info();
|
2014-10-27 22:49:46 +00:00
|
|
|
const std::vector<FileMetaData*>& l0 = vstorage->LevelFiles(0);
|
2015-12-15 23:26:20 +00:00
|
|
|
for (size_t i = 0; i < l0.size(); ++i) {
|
2015-08-19 23:05:51 +00:00
|
|
|
if (!l0_iters_[i]) {
|
|
|
|
continue;
|
|
|
|
}
|
2022-10-10 22:48:48 +00:00
|
|
|
if (seek_after_async_io) {
|
|
|
|
if (!l0_iters_[i]->status().IsTryAgain()) {
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
if (seek_to_first) {
|
|
|
|
l0_iters_[i]->SeekToFirst();
|
|
|
|
} else {
|
2021-03-26 04:17:17 +00:00
|
|
|
// If the target key passes over the largest key, we are sure Next()
|
2014-05-30 21:31:55 +00:00
|
|
|
// won't go over this file.
|
2022-10-10 22:48:48 +00:00
|
|
|
if (seek_after_async_io == false &&
|
|
|
|
user_comparator_->Compare(target_user_key,
|
2019-04-16 18:32:03 +00:00
|
|
|
l0[i]->largest.user_key()) > 0) {
|
2015-08-19 23:05:51 +00:00
|
|
|
if (read_options_.iterate_upper_bound != nullptr) {
|
|
|
|
has_iter_trimmed_for_upper_bound_ = true;
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(l0_iters_[i]);
|
2015-08-19 23:05:51 +00:00
|
|
|
l0_iters_[i] = nullptr;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
continue;
|
|
|
|
}
|
|
|
|
l0_iters_[i]->Seek(internal_key);
|
|
|
|
}
|
2014-08-29 21:32:37 +00:00
|
|
|
|
2022-10-10 22:48:48 +00:00
|
|
|
if (l0_iters_[i]->status().IsTryAgain()) {
|
|
|
|
assert(!seek_after_async_io);
|
|
|
|
continue;
|
|
|
|
} else if (!l0_iters_[i]->status().ok()) {
|
2014-09-26 21:20:24 +00:00
|
|
|
immutable_status_ = l0_iters_[i]->status();
|
2018-02-13 00:57:56 +00:00
|
|
|
} else if (l0_iters_[i]->Valid() &&
|
|
|
|
!IsOverUpperBound(l0_iters_[i]->key())) {
|
|
|
|
immutable_min_heap_.push(l0_iters_[i]);
|
|
|
|
} else {
|
|
|
|
has_iter_trimmed_for_upper_bound_ = true;
|
|
|
|
DeleteIterator(l0_iters_[i]);
|
|
|
|
l0_iters_[i] = nullptr;
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
}
|
2014-07-08 18:40:42 +00:00
|
|
|
|
2014-11-04 01:45:55 +00:00
|
|
|
for (int32_t level = 1; level < vstorage->num_levels(); ++level) {
|
2014-10-28 17:08:41 +00:00
|
|
|
const std::vector<FileMetaData*>& level_files =
|
2014-10-27 22:49:46 +00:00
|
|
|
vstorage->LevelFiles(level);
|
2014-10-28 17:08:41 +00:00
|
|
|
if (level_files.empty()) {
|
2014-05-30 21:31:55 +00:00
|
|
|
continue;
|
|
|
|
}
|
2015-08-19 23:05:51 +00:00
|
|
|
if (level_iters_[level - 1] == nullptr) {
|
|
|
|
continue;
|
|
|
|
}
|
2022-10-10 22:48:48 +00:00
|
|
|
|
|
|
|
if (seek_after_async_io) {
|
|
|
|
if (!level_iters_[level - 1]->status().IsTryAgain()) {
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
uint32_t f_idx = 0;
|
2022-10-10 22:48:48 +00:00
|
|
|
if (!seek_to_first && !seek_after_async_io) {
|
2016-10-28 17:25:39 +00:00
|
|
|
f_idx = FindFileInRange(level_files, internal_key, 0,
|
|
|
|
static_cast<uint32_t>(level_files.size()));
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
2014-07-08 18:40:42 +00:00
|
|
|
|
|
|
|
// Seek
|
2022-10-10 22:48:48 +00:00
|
|
|
if (seek_after_async_io || f_idx < level_files.size()) {
|
|
|
|
if (!seek_after_async_io) {
|
|
|
|
level_iters_[level - 1]->SetFileIndex(f_idx);
|
|
|
|
}
|
|
|
|
seek_to_first ? level_iters_[level - 1]->SeekToFirst()
|
|
|
|
: level_iters_[level - 1]->Seek(internal_key);
|
2014-08-29 21:32:37 +00:00
|
|
|
|
2022-10-10 22:48:48 +00:00
|
|
|
if (level_iters_[level - 1]->status().IsTryAgain()) {
|
|
|
|
assert(!seek_after_async_io);
|
|
|
|
continue;
|
|
|
|
} else if (!level_iters_[level - 1]->status().ok()) {
|
2014-09-26 21:20:24 +00:00
|
|
|
immutable_status_ = level_iters_[level - 1]->status();
|
2018-02-13 00:57:56 +00:00
|
|
|
} else if (level_iters_[level - 1]->Valid() &&
|
|
|
|
!IsOverUpperBound(level_iters_[level - 1]->key())) {
|
|
|
|
immutable_min_heap_.push(level_iters_[level - 1]);
|
|
|
|
} else {
|
|
|
|
// Nothing in this level is interesting. Remove.
|
|
|
|
has_iter_trimmed_for_upper_bound_ = true;
|
|
|
|
DeleteIterator(level_iters_[level - 1]);
|
|
|
|
level_iters_[level - 1] = nullptr;
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
if (seek_to_first) {
|
2014-05-30 21:31:55 +00:00
|
|
|
is_prev_set_ = false;
|
|
|
|
} else {
|
2017-04-04 21:17:16 +00:00
|
|
|
prev_key_.SetInternalKey(internal_key);
|
2014-05-30 21:31:55 +00:00
|
|
|
is_prev_set_ = true;
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
is_prev_inclusive_ = true;
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
2015-08-31 23:44:34 +00:00
|
|
|
|
|
|
|
TEST_SYNC_POINT_CALLBACK("ForwardIterator::SeekInternal:Immutable", this);
|
ForwardIterator seek bugfix
Summary:
If `NeedToSeekImmutable()` returns false, `SeekInternal()` won't reset the
contents of `immutable_min_heap_`. However, since it calls `UpdateCurrent()`
unconditionally, if `current_` is one of immutable iterators (previously popped
from `immutable_min_heap_`), `UpdateCurrent()` will overwrite it. As a result,
if old `current_` in fact pointed to the smallest entry, forward iterator will
skip some records.
Fix implemented in this diff pushes `current_` back to `immutable_min_heap_`
before calling `UpdateCurrent()`.
Test Plan:
New unit test (courtesy of @lovro):
$ ROCKSDB_TESTS=TailingIteratorSeekToSame ./db_test
Reviewers: igor, dhruba, haobo, ljin
Reviewed By: ljin
Subscribers: lovro, leveldb
Differential Revision: https://reviews.facebook.net/D19653
2014-07-10 22:14:24 +00:00
|
|
|
} else if (current_ && current_ != mutable_iter_) {
|
|
|
|
// current_ is one of immutable iterators, push it back to the heap
|
|
|
|
immutable_min_heap_.push(current_);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
2022-10-10 22:48:48 +00:00
|
|
|
// For async_io, it should be updated when seek_after_async_io is true (in
|
|
|
|
// second call).
|
|
|
|
if (seek_to_first || !read_options_.async_io || seek_after_async_io) {
|
|
|
|
UpdateCurrent();
|
|
|
|
}
|
2015-08-25 20:40:58 +00:00
|
|
|
TEST_SYNC_POINT_CALLBACK("ForwardIterator::SeekInternal:Return", this);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
void ForwardIterator::Next() {
|
|
|
|
assert(valid_);
|
2015-08-18 21:40:06 +00:00
|
|
|
bool update_prev_key = false;
|
2014-05-30 21:31:55 +00:00
|
|
|
|
2022-11-02 21:34:24 +00:00
|
|
|
if (sv_ == nullptr || sv_->version_number != cfd_->GetSuperVersionNumber()) {
|
2014-05-30 21:31:55 +00:00
|
|
|
std::string current_key = key().ToString();
|
|
|
|
Slice old_key(current_key.data(), current_key.size());
|
|
|
|
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
if (sv_ == nullptr) {
|
|
|
|
RebuildIterators(true);
|
|
|
|
} else {
|
|
|
|
RenewIterators();
|
|
|
|
}
|
2022-10-10 22:48:48 +00:00
|
|
|
|
|
|
|
SeekInternal(old_key, false, false);
|
|
|
|
if (read_options_.async_io) {
|
|
|
|
SeekInternal(old_key, false, true);
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
if (!valid_ || key().compare(old_key) != 0) {
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
} else if (current_ != mutable_iter_) {
|
|
|
|
// It is going to advance immutable iterator
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
|
|
|
|
if (is_prev_set_ && prefix_extractor_) {
|
|
|
|
// advance prev_key_ to current_ only if they share the same prefix
|
|
|
|
update_prev_key =
|
2017-04-04 21:17:16 +00:00
|
|
|
prefix_extractor_->Transform(prev_key_.GetUserKey())
|
|
|
|
.compare(prefix_extractor_->Transform(current_->key())) == 0;
|
2015-08-18 21:40:06 +00:00
|
|
|
} else {
|
|
|
|
update_prev_key = true;
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
if (update_prev_key) {
|
2017-04-04 21:17:16 +00:00
|
|
|
prev_key_.SetInternalKey(current_->key());
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
is_prev_set_ = true;
|
|
|
|
is_prev_inclusive_ = false;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
current_->Next();
|
2014-08-29 21:32:37 +00:00
|
|
|
if (current_ != mutable_iter_) {
|
2014-09-26 21:20:24 +00:00
|
|
|
if (!current_->status().ok()) {
|
|
|
|
immutable_status_ = current_->status();
|
2015-08-19 23:05:51 +00:00
|
|
|
} else if ((current_->Valid()) && (!IsOverUpperBound(current_->key()))) {
|
2014-08-29 21:32:37 +00:00
|
|
|
immutable_min_heap_.push(current_);
|
2015-08-19 23:05:51 +00:00
|
|
|
} else {
|
|
|
|
if ((current_->Valid()) && (IsOverUpperBound(current_->key()))) {
|
|
|
|
// remove the current iterator
|
|
|
|
DeleteCurrentIter();
|
|
|
|
current_ = nullptr;
|
|
|
|
}
|
2015-09-04 21:28:45 +00:00
|
|
|
if (update_prev_key) {
|
2017-04-04 21:17:16 +00:00
|
|
|
mutable_iter_->Seek(prev_key_.GetInternalKey());
|
2015-08-19 23:05:51 +00:00
|
|
|
}
|
2014-08-29 21:32:37 +00:00
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
UpdateCurrent();
|
2015-08-25 20:40:58 +00:00
|
|
|
TEST_SYNC_POINT_CALLBACK("ForwardIterator::Next:Return", this);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
Slice ForwardIterator::key() const {
|
|
|
|
assert(valid_);
|
|
|
|
return current_->key();
|
|
|
|
}
|
|
|
|
|
Support returning write unix time in iterator property (#12428)
Summary:
This PR adds support to return data's approximate unix write time in the iterator property API. The general implementation is:
1) If the entry comes from a SST file, the sequence number to time mapping recorded in that file's table properties will be used to deduce the entry's write time from its sequence number. If no such recording is available, `std::numeric_limits<uint64_t>::max()` is returned to indicate the write time is unknown except if the entry's sequence number is zero, in which case, 0 is returned. This also means that even if `preclude_last_level_data_seconds` and `preserve_internal_time_seconds` can be toggled off between DB reopens, as long as the SST file's table property has the mapping available, the entry's write time can be deduced and returned.
2) If the entry comes from memtable, we will use the DB's sequence number to write time mapping to do similar things. A copy of the DB's seqno to write time mapping is kept in SuperVersion to allow iterators to have lock free access. This also means a new `SuperVersion` is installed each time DB's seqno to time mapping updates, which is originally proposed by Peter in https://github.com/facebook/rocksdb/issues/11928 . Similarly, if the feature is not enabled, `std::numeric_limits<uint64_t>::max()` is returned to indicate the write time is unknown.
Needed follow up:
1) The write time for `kTypeValuePreferredSeqno` should be special cased, where it's already specified by the user, so we can directly return it.
2) Flush job can be updated to use DB's seqno to time mapping copy in the SuperVersion.
3) Handle the case when `TimedPut` is called with a write time that is `std::numeric_limits<uint64_t>::max()`. We can make it a regular `Put`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12428
Test Plan: Added unit test
Reviewed By: pdillinger
Differential Revision: D54967067
Pulled By: jowlyzhang
fbshipit-source-id: c795b1b7ec142e09e53f2ed3461cf719833cb37a
2024-03-15 22:37:37 +00:00
|
|
|
uint64_t ForwardIterator::write_unix_time() const {
|
|
|
|
assert(valid_);
|
|
|
|
return current_->write_unix_time();
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
Slice ForwardIterator::value() const {
|
|
|
|
assert(valid_);
|
|
|
|
return current_->value();
|
|
|
|
}
|
|
|
|
|
|
|
|
Status ForwardIterator::status() const {
|
|
|
|
if (!status_.ok()) {
|
|
|
|
return status_;
|
|
|
|
} else if (!mutable_iter_->status().ok()) {
|
|
|
|
return mutable_iter_->status();
|
|
|
|
}
|
2014-07-10 00:46:18 +00:00
|
|
|
|
2014-09-26 21:20:24 +00:00
|
|
|
return immutable_status_;
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
bool ForwardIterator::PrepareValue() {
|
|
|
|
assert(valid_);
|
|
|
|
if (current_->PrepareValue()) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
|
|
|
assert(!current_->Valid());
|
|
|
|
assert(!current_->status().ok());
|
2022-11-02 21:34:24 +00:00
|
|
|
assert(current_ != mutable_iter_); // memtable iterator can't fail
|
Properly report IO errors when IndexType::kBinarySearchWithFirstKey is used (#6621)
Summary:
Context: Index type `kBinarySearchWithFirstKey` added the ability for sst file iterator to sometimes report a key from index without reading the corresponding data block. This is useful when sst blocks are cut at some meaningful boundaries (e.g. one block per key prefix), and many seeks land between blocks (e.g. for each prefix, the ranges of keys in different sst files are nearly disjoint, so a typical seek needs to read a data block from only one file even if all files have the prefix). But this added a new error condition, which rocksdb code was really not equipped to deal with: `InternalIterator::value()` may fail with an IO error or Status::Incomplete, but it's just a method returning a Slice, with no way to report error instead. Before this PR, this type of error wasn't handled at all (an empty slice was returned), and kBinarySearchWithFirstKey implementation was considered a prototype.
Now that we (LogDevice) have experimented with kBinarySearchWithFirstKey for a while and confirmed that it's really useful, this PR is adding the missing error handling.
It's a pretty inconvenient situation implementation-wise. The error needs to be reported from InternalIterator when trying to access value. But there are ~700 call sites of `InternalIterator::value()`, most of which either can't hit the error condition (because the iterator is reading from memtable or from index or something) or wouldn't benefit from the deferred loading of the value (e.g. compaction iterator that reads all values anyway). Adding error handling to all these call sites would needlessly bloat the code. So instead I made the deferred value loading optional: only the call sites that may use deferred loading have to call the new method `PrepareValue()` before calling `value()`. The feature is enabled with a new bool argument `allow_unprepared_value` to a bunch of methods that create iterators (it wouldn't make sense to put it in ReadOptions because it's completely internal to iterators, with virtually no user-visible effect). Lmk if you have better ideas.
Note that the deferred value loading only happens for *internal* iterators. The user-visible iterator (DBIter) always prepares the value before returning from Seek/Next/etc. We could go further and add an API to defer that value loading too, but that's most likely not useful for LogDevice, so it doesn't seem worth the complexity for now.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6621
Test Plan: make -j5 check . Will also deploy to some logdevice test clusters and look at stats.
Reviewed By: siying
Differential Revision: D20786930
Pulled By: al13n321
fbshipit-source-id: 6da77d918bad3780522e918f17f4d5513d3e99ee
2020-04-16 00:37:23 +00:00
|
|
|
assert(immutable_status_.ok());
|
|
|
|
|
|
|
|
valid_ = false;
|
|
|
|
immutable_status_ = current_->status();
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
2016-03-01 02:38:03 +00:00
|
|
|
Status ForwardIterator::GetProperty(std::string prop_name, std::string* prop) {
|
|
|
|
assert(prop != nullptr);
|
2016-03-03 21:18:56 +00:00
|
|
|
if (prop_name == "rocksdb.iterator.super-version-number") {
|
2022-05-06 20:03:58 +00:00
|
|
|
*prop = std::to_string(sv_->version_number);
|
2016-03-01 02:38:03 +00:00
|
|
|
return Status::OK();
|
|
|
|
}
|
2024-01-30 00:31:09 +00:00
|
|
|
return Status::InvalidArgument("Unrecognized property: " + prop_name);
|
2016-03-01 02:38:03 +00:00
|
|
|
}
|
|
|
|
|
2016-08-12 02:10:16 +00:00
|
|
|
void ForwardIterator::SetPinnedItersMgr(
|
|
|
|
PinnedIteratorsManager* pinned_iters_mgr) {
|
|
|
|
pinned_iters_mgr_ = pinned_iters_mgr;
|
|
|
|
UpdateChildrenPinnedItersMgr();
|
|
|
|
}
|
|
|
|
|
|
|
|
void ForwardIterator::UpdateChildrenPinnedItersMgr() {
|
|
|
|
// Set PinnedIteratorsManager for mutable memtable iterator.
|
|
|
|
if (mutable_iter_) {
|
|
|
|
mutable_iter_->SetPinnedItersMgr(pinned_iters_mgr_);
|
|
|
|
}
|
|
|
|
|
|
|
|
// Set PinnedIteratorsManager for immutable memtable iterators.
|
|
|
|
for (InternalIterator* child_iter : imm_iters_) {
|
|
|
|
if (child_iter) {
|
|
|
|
child_iter->SetPinnedItersMgr(pinned_iters_mgr_);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
// Set PinnedIteratorsManager for L0 files iterators.
|
|
|
|
for (InternalIterator* child_iter : l0_iters_) {
|
|
|
|
if (child_iter) {
|
|
|
|
child_iter->SetPinnedItersMgr(pinned_iters_mgr_);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
// Set PinnedIteratorsManager for L1+ levels iterators.
|
2018-02-13 21:44:22 +00:00
|
|
|
for (ForwardLevelIterator* child_iter : level_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
if (child_iter) {
|
|
|
|
child_iter->SetPinnedItersMgr(pinned_iters_mgr_);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
bool ForwardIterator::IsKeyPinned() const {
|
|
|
|
return pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled() &&
|
|
|
|
current_->IsKeyPinned();
|
|
|
|
}
|
|
|
|
|
|
|
|
bool ForwardIterator::IsValuePinned() const {
|
|
|
|
return pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled() &&
|
|
|
|
current_->IsValuePinned();
|
|
|
|
}
|
|
|
|
|
2014-10-23 22:34:21 +00:00
|
|
|
void ForwardIterator::RebuildIterators(bool refresh_sv) {
|
2014-05-30 21:31:55 +00:00
|
|
|
// Clean up
|
2014-10-23 22:34:21 +00:00
|
|
|
Cleanup(refresh_sv);
|
|
|
|
if (refresh_sv) {
|
|
|
|
// New
|
2019-12-17 21:20:42 +00:00
|
|
|
sv_ = cfd_->GetReferencedSuperVersion(db_);
|
2014-10-23 22:34:21 +00:00
|
|
|
}
|
2018-12-18 01:26:56 +00:00
|
|
|
ReadRangeDelAggregator range_del_agg(&cfd_->internal_comparator(),
|
|
|
|
kMaxSequenceNumber /* upper_bound */);
|
Support returning write unix time in iterator property (#12428)
Summary:
This PR adds support to return data's approximate unix write time in the iterator property API. The general implementation is:
1) If the entry comes from a SST file, the sequence number to time mapping recorded in that file's table properties will be used to deduce the entry's write time from its sequence number. If no such recording is available, `std::numeric_limits<uint64_t>::max()` is returned to indicate the write time is unknown except if the entry's sequence number is zero, in which case, 0 is returned. This also means that even if `preclude_last_level_data_seconds` and `preserve_internal_time_seconds` can be toggled off between DB reopens, as long as the SST file's table property has the mapping available, the entry's write time can be deduced and returned.
2) If the entry comes from memtable, we will use the DB's sequence number to write time mapping to do similar things. A copy of the DB's seqno to write time mapping is kept in SuperVersion to allow iterators to have lock free access. This also means a new `SuperVersion` is installed each time DB's seqno to time mapping updates, which is originally proposed by Peter in https://github.com/facebook/rocksdb/issues/11928 . Similarly, if the feature is not enabled, `std::numeric_limits<uint64_t>::max()` is returned to indicate the write time is unknown.
Needed follow up:
1) The write time for `kTypeValuePreferredSeqno` should be special cased, where it's already specified by the user, so we can directly return it.
2) Flush job can be updated to use DB's seqno to time mapping copy in the SuperVersion.
3) Handle the case when `TimedPut` is called with a write time that is `std::numeric_limits<uint64_t>::max()`. We can make it a regular `Put`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12428
Test Plan: Added unit test
Reviewed By: pdillinger
Differential Revision: D54967067
Pulled By: jowlyzhang
fbshipit-source-id: c795b1b7ec142e09e53f2ed3461cf719833cb37a
2024-03-15 22:37:37 +00:00
|
|
|
UnownedPtr<const SeqnoToTimeMapping> seqno_to_time_mapping =
|
|
|
|
sv_->GetSeqnoToTimeMapping();
|
|
|
|
mutable_iter_ =
|
|
|
|
sv_->mem->NewIterator(read_options_, seqno_to_time_mapping, &arena_);
|
|
|
|
sv_->imm->AddIterators(read_options_, seqno_to_time_mapping, &imm_iters_,
|
|
|
|
&arena_);
|
2017-01-23 21:30:14 +00:00
|
|
|
if (!read_options_.ignore_range_deletions) {
|
2018-11-28 23:26:56 +00:00
|
|
|
std::unique_ptr<FragmentedRangeTombstoneIterator> range_del_iter(
|
|
|
|
sv_->mem->NewRangeTombstoneIterator(
|
2022-08-05 19:02:33 +00:00
|
|
|
read_options_, sv_->current->version_set()->LastSequence(),
|
|
|
|
false /* immutable_memtable */));
|
2018-11-28 23:26:56 +00:00
|
|
|
range_del_agg.AddTombstones(std::move(range_del_iter));
|
2020-10-02 20:33:50 +00:00
|
|
|
// Always return Status::OK().
|
2020-10-22 03:16:30 +00:00
|
|
|
Status temp_s = sv_->imm->AddRangeTombstoneIterators(read_options_, &arena_,
|
|
|
|
&range_del_agg);
|
|
|
|
assert(temp_s.ok());
|
2017-01-23 21:30:14 +00:00
|
|
|
}
|
2015-08-25 20:40:58 +00:00
|
|
|
has_iter_trimmed_for_upper_bound_ = false;
|
2014-10-27 22:49:46 +00:00
|
|
|
|
2014-10-31 15:48:19 +00:00
|
|
|
const auto* vstorage = sv_->current->storage_info();
|
2014-10-27 22:49:46 +00:00
|
|
|
const auto& l0_files = vstorage->LevelFiles(0);
|
2014-05-30 21:31:55 +00:00
|
|
|
l0_iters_.reserve(l0_files.size());
|
|
|
|
for (const auto* l0 : l0_files) {
|
2015-08-19 23:05:51 +00:00
|
|
|
if ((read_options_.iterate_upper_bound != nullptr) &&
|
|
|
|
cfd_->internal_comparator().user_comparator()->Compare(
|
|
|
|
l0->smallest.user_key(), *read_options_.iterate_upper_bound) > 0) {
|
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
|
|
|
// No need to set has_iter_trimmed_for_upper_bound_: this ForwardIterator
|
|
|
|
// will never be interested in files with smallest key above
|
|
|
|
// iterate_upper_bound, since iterate_upper_bound can't be changed.
|
2015-08-19 23:05:51 +00:00
|
|
|
l0_iters_.push_back(nullptr);
|
|
|
|
continue;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
l0_iters_.push_back(cfd_->table_cache()->NewIterator(
|
2018-07-14 00:34:54 +00:00
|
|
|
read_options_, *cfd_->soptions(), cfd_->internal_comparator(), *l0,
|
2018-05-21 21:33:55 +00:00
|
|
|
read_options_.ignore_range_deletions ? nullptr : &range_del_agg,
|
2022-01-21 19:36:36 +00:00
|
|
|
sv_->mutable_cf_options.prefix_extractor,
|
2019-06-20 21:28:22 +00:00
|
|
|
/*table_reader_ptr=*/nullptr, /*file_read_hist=*/nullptr,
|
|
|
|
TableReaderCaller::kUserIterator, /*arena=*/nullptr,
|
|
|
|
/*skip_filters=*/false, /*level=*/-1,
|
2020-06-09 23:49:07 +00:00
|
|
|
MaxFileSizeForL0MetaPin(sv_->mutable_cf_options),
|
2019-06-20 21:28:22 +00:00
|
|
|
/*smallest_compaction_key=*/nullptr,
|
2023-04-25 19:08:23 +00:00
|
|
|
/*largest_compaction_key=*/nullptr, allow_unprepared_value_,
|
|
|
|
sv_->mutable_cf_options.block_protection_bytes_per_key));
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
2022-01-21 19:36:36 +00:00
|
|
|
BuildLevelIterators(vstorage, sv_);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
current_ = nullptr;
|
|
|
|
is_prev_set_ = false;
|
2016-08-12 02:10:16 +00:00
|
|
|
|
|
|
|
UpdateChildrenPinnedItersMgr();
|
2017-01-23 21:30:14 +00:00
|
|
|
if (!range_del_agg.IsEmpty()) {
|
|
|
|
status_ = Status::NotSupported(
|
|
|
|
"Range tombstones unsupported with ForwardIterator");
|
|
|
|
valid_ = false;
|
|
|
|
}
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
void ForwardIterator::RenewIterators() {
|
|
|
|
SuperVersion* svnew;
|
|
|
|
assert(sv_);
|
2019-12-17 21:20:42 +00:00
|
|
|
svnew = cfd_->GetReferencedSuperVersion(db_);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
|
|
|
|
if (mutable_iter_ != nullptr) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(mutable_iter_, true /* is_arena */);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
for (auto* m : imm_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(m, true /* is_arena */);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
imm_iters_.clear();
|
|
|
|
|
Support returning write unix time in iterator property (#12428)
Summary:
This PR adds support to return data's approximate unix write time in the iterator property API. The general implementation is:
1) If the entry comes from a SST file, the sequence number to time mapping recorded in that file's table properties will be used to deduce the entry's write time from its sequence number. If no such recording is available, `std::numeric_limits<uint64_t>::max()` is returned to indicate the write time is unknown except if the entry's sequence number is zero, in which case, 0 is returned. This also means that even if `preclude_last_level_data_seconds` and `preserve_internal_time_seconds` can be toggled off between DB reopens, as long as the SST file's table property has the mapping available, the entry's write time can be deduced and returned.
2) If the entry comes from memtable, we will use the DB's sequence number to write time mapping to do similar things. A copy of the DB's seqno to write time mapping is kept in SuperVersion to allow iterators to have lock free access. This also means a new `SuperVersion` is installed each time DB's seqno to time mapping updates, which is originally proposed by Peter in https://github.com/facebook/rocksdb/issues/11928 . Similarly, if the feature is not enabled, `std::numeric_limits<uint64_t>::max()` is returned to indicate the write time is unknown.
Needed follow up:
1) The write time for `kTypeValuePreferredSeqno` should be special cased, where it's already specified by the user, so we can directly return it.
2) Flush job can be updated to use DB's seqno to time mapping copy in the SuperVersion.
3) Handle the case when `TimedPut` is called with a write time that is `std::numeric_limits<uint64_t>::max()`. We can make it a regular `Put`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/12428
Test Plan: Added unit test
Reviewed By: pdillinger
Differential Revision: D54967067
Pulled By: jowlyzhang
fbshipit-source-id: c795b1b7ec142e09e53f2ed3461cf719833cb37a
2024-03-15 22:37:37 +00:00
|
|
|
UnownedPtr<const SeqnoToTimeMapping> seqno_to_time_mapping =
|
|
|
|
svnew->GetSeqnoToTimeMapping();
|
|
|
|
mutable_iter_ =
|
|
|
|
svnew->mem->NewIterator(read_options_, seqno_to_time_mapping, &arena_);
|
|
|
|
svnew->imm->AddIterators(read_options_, seqno_to_time_mapping, &imm_iters_,
|
|
|
|
&arena_);
|
2018-12-18 01:26:56 +00:00
|
|
|
ReadRangeDelAggregator range_del_agg(&cfd_->internal_comparator(),
|
|
|
|
kMaxSequenceNumber /* upper_bound */);
|
2017-01-23 21:30:14 +00:00
|
|
|
if (!read_options_.ignore_range_deletions) {
|
2018-11-28 23:26:56 +00:00
|
|
|
std::unique_ptr<FragmentedRangeTombstoneIterator> range_del_iter(
|
|
|
|
svnew->mem->NewRangeTombstoneIterator(
|
2022-08-05 19:02:33 +00:00
|
|
|
read_options_, sv_->current->version_set()->LastSequence(),
|
|
|
|
false /* immutable_memtable */));
|
2018-11-28 23:26:56 +00:00
|
|
|
range_del_agg.AddTombstones(std::move(range_del_iter));
|
2020-10-02 20:33:50 +00:00
|
|
|
// Always return Status::OK().
|
2020-10-22 03:16:30 +00:00
|
|
|
Status temp_s = svnew->imm->AddRangeTombstoneIterators(
|
|
|
|
read_options_, &arena_, &range_del_agg);
|
|
|
|
assert(temp_s.ok());
|
2017-01-23 21:30:14 +00:00
|
|
|
}
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
|
|
|
|
const auto* vstorage = sv_->current->storage_info();
|
|
|
|
const auto& l0_files = vstorage->LevelFiles(0);
|
|
|
|
const auto* vstorage_new = svnew->current->storage_info();
|
|
|
|
const auto& l0_files_new = vstorage_new->LevelFiles(0);
|
2015-12-15 23:26:20 +00:00
|
|
|
size_t iold, inew;
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
bool found;
|
|
|
|
std::vector<InternalIterator*> l0_iters_new;
|
|
|
|
l0_iters_new.reserve(l0_files_new.size());
|
|
|
|
|
|
|
|
for (inew = 0; inew < l0_files_new.size(); inew++) {
|
|
|
|
found = false;
|
|
|
|
for (iold = 0; iold < l0_files.size(); iold++) {
|
|
|
|
if (l0_files[iold] == l0_files_new[inew]) {
|
|
|
|
found = true;
|
|
|
|
break;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
if (found) {
|
|
|
|
if (l0_iters_[iold] == nullptr) {
|
|
|
|
l0_iters_new.push_back(nullptr);
|
|
|
|
TEST_SYNC_POINT_CALLBACK("ForwardIterator::RenewIterators:Null", this);
|
|
|
|
} else {
|
|
|
|
l0_iters_new.push_back(l0_iters_[iold]);
|
|
|
|
l0_iters_[iold] = nullptr;
|
|
|
|
TEST_SYNC_POINT_CALLBACK("ForwardIterator::RenewIterators:Copy", this);
|
|
|
|
}
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
l0_iters_new.push_back(cfd_->table_cache()->NewIterator(
|
|
|
|
read_options_, *cfd_->soptions(), cfd_->internal_comparator(),
|
2018-07-14 00:34:54 +00:00
|
|
|
*l0_files_new[inew],
|
2018-05-21 21:33:55 +00:00
|
|
|
read_options_.ignore_range_deletions ? nullptr : &range_del_agg,
|
2022-01-21 19:36:36 +00:00
|
|
|
svnew->mutable_cf_options.prefix_extractor,
|
2019-06-20 21:28:22 +00:00
|
|
|
/*table_reader_ptr=*/nullptr, /*file_read_hist=*/nullptr,
|
|
|
|
TableReaderCaller::kUserIterator, /*arena=*/nullptr,
|
|
|
|
/*skip_filters=*/false, /*level=*/-1,
|
2020-06-09 23:49:07 +00:00
|
|
|
MaxFileSizeForL0MetaPin(svnew->mutable_cf_options),
|
2019-06-20 21:28:22 +00:00
|
|
|
/*smallest_compaction_key=*/nullptr,
|
2023-04-25 19:08:23 +00:00
|
|
|
/*largest_compaction_key=*/nullptr, allow_unprepared_value_,
|
|
|
|
svnew->mutable_cf_options.block_protection_bytes_per_key));
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
for (auto* f : l0_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(f);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
l0_iters_.clear();
|
|
|
|
l0_iters_ = l0_iters_new;
|
|
|
|
|
|
|
|
for (auto* l : level_iters_) {
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(l);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
2015-11-17 18:27:51 +00:00
|
|
|
level_iters_.clear();
|
2022-01-21 19:36:36 +00:00
|
|
|
BuildLevelIterators(vstorage_new, svnew);
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
current_ = nullptr;
|
|
|
|
is_prev_set_ = false;
|
|
|
|
SVCleanup();
|
|
|
|
sv_ = svnew;
|
2016-08-12 02:10:16 +00:00
|
|
|
|
|
|
|
UpdateChildrenPinnedItersMgr();
|
2017-01-23 21:30:14 +00:00
|
|
|
if (!range_del_agg.IsEmpty()) {
|
|
|
|
status_ = Status::NotSupported(
|
|
|
|
"Range tombstones unsupported with ForwardIterator");
|
|
|
|
valid_ = false;
|
|
|
|
}
|
Reuse file iterators in tailing iterator when memtable is flushed
Summary:
Under a tailing workload, there were increased block cache
misses when a memtable was flushed because we were rebuilding iterators
in that case since the version set changed. This was exacerbated in the
case of iterate_upper_bound, since file iterators which were over the
iterate_upper_bound would have been deleted and are now brought back as
part of the Rebuild, only to be deleted again. We now renew the iterators
and only build iterators for files which are added and delete file
iterators for files which are deleted.
Refer to https://reviews.facebook.net/D50463 for previous version
Test Plan: DBTestTailingIterator.TailingIteratorTrimSeekToNext
Reviewers: anthony, IslamAbdelRahman, igor, tnovak, yhchiang, sdong
Reviewed By: sdong
Subscribers: yhchiang, march, dhruba, leveldb, lovro
Differential Revision: https://reviews.facebook.net/D50679
2015-11-13 23:50:59 +00:00
|
|
|
}
|
|
|
|
|
2022-01-21 19:36:36 +00:00
|
|
|
void ForwardIterator::BuildLevelIterators(const VersionStorageInfo* vstorage,
|
|
|
|
SuperVersion* sv) {
|
2015-11-17 18:27:51 +00:00
|
|
|
level_iters_.reserve(vstorage->num_levels() - 1);
|
2014-11-04 01:45:55 +00:00
|
|
|
for (int32_t level = 1; level < vstorage->num_levels(); ++level) {
|
2014-10-27 22:49:46 +00:00
|
|
|
const auto& level_files = vstorage->LevelFiles(level);
|
2015-08-19 23:05:51 +00:00
|
|
|
if ((level_files.empty()) ||
|
|
|
|
((read_options_.iterate_upper_bound != nullptr) &&
|
|
|
|
(user_comparator_->Compare(*read_options_.iterate_upper_bound,
|
|
|
|
level_files[0]->smallest.user_key()) <
|
|
|
|
0))) {
|
2015-11-17 18:27:51 +00:00
|
|
|
level_iters_.push_back(nullptr);
|
2015-08-25 20:40:58 +00:00
|
|
|
if (!level_files.empty()) {
|
|
|
|
has_iter_trimmed_for_upper_bound_ = true;
|
|
|
|
}
|
2014-05-30 21:31:55 +00:00
|
|
|
} else {
|
2018-05-21 21:33:55 +00:00
|
|
|
level_iters_.push_back(new ForwardLevelIterator(
|
|
|
|
cfd_, read_options_, level_files,
|
2023-04-25 19:08:23 +00:00
|
|
|
sv->mutable_cf_options.prefix_extractor, allow_unprepared_value_,
|
|
|
|
sv->mutable_cf_options.block_protection_bytes_per_key));
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2014-08-29 21:32:37 +00:00
|
|
|
void ForwardIterator::ResetIncompleteIterators() {
|
2014-10-31 15:48:19 +00:00
|
|
|
const auto& l0_files = sv_->current->storage_info()->LevelFiles(0);
|
2015-12-15 23:26:20 +00:00
|
|
|
for (size_t i = 0; i < l0_iters_.size(); ++i) {
|
2014-08-29 21:32:37 +00:00
|
|
|
assert(i < l0_files.size());
|
2015-08-25 20:38:35 +00:00
|
|
|
if (!l0_iters_[i] || !l0_iters_[i]->status().IsIncomplete()) {
|
2014-08-29 21:32:37 +00:00
|
|
|
continue;
|
|
|
|
}
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(l0_iters_[i]);
|
2014-08-29 21:32:37 +00:00
|
|
|
l0_iters_[i] = cfd_->table_cache()->NewIterator(
|
|
|
|
read_options_, *cfd_->soptions(), cfd_->internal_comparator(),
|
2019-06-20 21:28:22 +00:00
|
|
|
*l0_files[i], /*range_del_agg=*/nullptr,
|
2022-01-21 19:36:36 +00:00
|
|
|
sv_->mutable_cf_options.prefix_extractor,
|
2019-06-20 21:28:22 +00:00
|
|
|
/*table_reader_ptr=*/nullptr, /*file_read_hist=*/nullptr,
|
|
|
|
TableReaderCaller::kUserIterator, /*arena=*/nullptr,
|
|
|
|
/*skip_filters=*/false, /*level=*/-1,
|
2020-06-09 23:49:07 +00:00
|
|
|
MaxFileSizeForL0MetaPin(sv_->mutable_cf_options),
|
2019-06-20 21:28:22 +00:00
|
|
|
/*smallest_compaction_key=*/nullptr,
|
2023-04-25 19:08:23 +00:00
|
|
|
/*largest_compaction_key=*/nullptr, allow_unprepared_value_,
|
|
|
|
sv_->mutable_cf_options.block_protection_bytes_per_key);
|
2016-08-12 02:10:16 +00:00
|
|
|
l0_iters_[i]->SetPinnedItersMgr(pinned_iters_mgr_);
|
2014-08-29 21:32:37 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
for (auto* level_iter : level_iters_) {
|
|
|
|
if (level_iter && level_iter->status().IsIncomplete()) {
|
|
|
|
level_iter->Reset();
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
current_ = nullptr;
|
|
|
|
is_prev_set_ = false;
|
|
|
|
}
|
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
void ForwardIterator::UpdateCurrent() {
|
|
|
|
if (immutable_min_heap_.empty() && !mutable_iter_->Valid()) {
|
|
|
|
current_ = nullptr;
|
|
|
|
} else if (immutable_min_heap_.empty()) {
|
|
|
|
current_ = mutable_iter_;
|
|
|
|
} else if (!mutable_iter_->Valid()) {
|
|
|
|
current_ = immutable_min_heap_.top();
|
|
|
|
immutable_min_heap_.pop();
|
|
|
|
} else {
|
|
|
|
current_ = immutable_min_heap_.top();
|
|
|
|
assert(current_ != nullptr);
|
|
|
|
assert(current_->Valid());
|
|
|
|
int cmp = cfd_->internal_comparator().InternalKeyComparator::Compare(
|
2014-06-10 16:57:26 +00:00
|
|
|
mutable_iter_->key(), current_->key());
|
2014-05-30 21:31:55 +00:00
|
|
|
assert(cmp != 0);
|
|
|
|
if (cmp > 0) {
|
|
|
|
immutable_min_heap_.pop();
|
|
|
|
} else {
|
|
|
|
current_ = mutable_iter_;
|
|
|
|
}
|
|
|
|
}
|
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
|
|
|
valid_ = current_ != nullptr && immutable_status_.ok();
|
2014-05-30 21:31:55 +00:00
|
|
|
if (!status_.ok()) {
|
|
|
|
status_ = Status::OK();
|
|
|
|
}
|
2015-08-31 23:44:34 +00:00
|
|
|
|
|
|
|
// Upper bound doesn't apply to the memtable iterator. We want Valid() to
|
|
|
|
// return false when all iterators are over iterate_upper_bound, but can't
|
|
|
|
// just set valid_ to false, as that would effectively disable the tailing
|
|
|
|
// optimization (Seek() would be called on all immutable iterators regardless
|
|
|
|
// of whether the target key is greater than prev_key_).
|
|
|
|
current_over_upper_bound_ = valid_ && IsOverUpperBound(current_->key());
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
bool ForwardIterator::NeedToSeekImmutable(const Slice& target) {
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
// We maintain the interval (prev_key_, immutable_min_heap_.top()->key())
|
|
|
|
// such that there are no records with keys within that range in
|
|
|
|
// immutable_min_heap_. Since immutable structures (SST files and immutable
|
|
|
|
// memtables) can't change in this version, we don't need to do a seek if
|
|
|
|
// 'target' belongs to that interval (immutable_min_heap_.top() is already
|
|
|
|
// at the correct position).
|
|
|
|
|
2014-09-26 21:20:24 +00:00
|
|
|
if (!valid_ || !current_ || !is_prev_set_ || !immutable_status_.ok()) {
|
2014-05-30 21:31:55 +00:00
|
|
|
return true;
|
|
|
|
}
|
2017-04-04 21:17:16 +00:00
|
|
|
Slice prev_key = prev_key_.GetInternalKey();
|
2014-05-30 21:31:55 +00:00
|
|
|
if (prefix_extractor_ && prefix_extractor_->Transform(target).compare(
|
2022-11-02 21:34:24 +00:00
|
|
|
prefix_extractor_->Transform(prev_key)) != 0) {
|
2014-05-30 21:31:55 +00:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
if (cfd_->internal_comparator().InternalKeyComparator::Compare(
|
2022-11-02 21:34:24 +00:00
|
|
|
prev_key, target) >= (is_prev_inclusive_ ? 1 : 0)) {
|
2014-05-30 21:31:55 +00:00
|
|
|
return true;
|
|
|
|
}
|
ForwardIterator: update prev_key_ only if prefix hasn't changed
Summary:
Since ForwardIterator is on a level below DBIter, the latter may call Next() on
it (e.g. in order to skip deletion markers). Since this also updates
`prev_key_`, it may prevent the Seek() optimization.
For example, assume that there's only one SST file and it contains the following
entries: 0101, 0201 (`ValueType::kTypeDeletion`, i.e. a tombstone record), 0201
(`kTypeValue`), 0202. Memtable is empty. `Seek(0102)` will result in `prev_key_`
being set to `0201` instead of `0102`, since `DBIter::Seek()` will call
`ForwardIterator::Next()` to skip record 0201. Therefore, when `Seek(0102)` is
called again, `NeedToSeekImmutable()` will return true.
This fix relies on `prefix_extractor_` to detect prefix changes. `prev_key_` is
only set to `current_->key()` as long as they have the same prefix.
I also made a small change to `NeedToSeekImmutable()` so it no longer returns
true when the db is empty (i.e. there's nothing but a memtable).
Test Plan:
$ TEST_TMPDIR=/dev/shm/rocksdbtest ROCKSDB_TESTS=TailingIterator ./db_test
Reviewers: sdong, igor, ljin
Reviewed By: ljin
Subscribers: leveldb
Differential Revision: https://reviews.facebook.net/D23823
2014-09-22 22:20:03 +00:00
|
|
|
|
|
|
|
if (immutable_min_heap_.empty() && current_ == mutable_iter_) {
|
|
|
|
// Nothing to seek on.
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
if (cfd_->internal_comparator().InternalKeyComparator::Compare(
|
2022-11-02 21:34:24 +00:00
|
|
|
target, current_ == mutable_iter_ ? immutable_min_heap_.top()->key()
|
|
|
|
: current_->key()) > 0) {
|
2014-05-30 21:31:55 +00:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
2015-08-19 23:05:51 +00:00
|
|
|
void ForwardIterator::DeleteCurrentIter() {
|
|
|
|
const VersionStorageInfo* vstorage = sv_->current->storage_info();
|
|
|
|
const std::vector<FileMetaData*>& l0 = vstorage->LevelFiles(0);
|
2015-12-15 23:26:20 +00:00
|
|
|
for (size_t i = 0; i < l0.size(); ++i) {
|
2015-08-19 23:05:51 +00:00
|
|
|
if (!l0_iters_[i]) {
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
if (l0_iters_[i] == current_) {
|
|
|
|
has_iter_trimmed_for_upper_bound_ = true;
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(l0_iters_[i]);
|
2015-08-19 23:05:51 +00:00
|
|
|
l0_iters_[i] = nullptr;
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
for (int32_t level = 1; level < vstorage->num_levels(); ++level) {
|
|
|
|
if (level_iters_[level - 1] == nullptr) {
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
if (level_iters_[level - 1] == current_) {
|
|
|
|
has_iter_trimmed_for_upper_bound_ = true;
|
2016-08-12 02:10:16 +00:00
|
|
|
DeleteIterator(level_iters_[level - 1]);
|
2015-08-19 23:05:51 +00:00
|
|
|
level_iters_[level - 1] = nullptr;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2015-09-04 21:28:45 +00:00
|
|
|
bool ForwardIterator::TEST_CheckDeletedIters(int* pdeleted_iters,
|
|
|
|
int* pnum_iters) {
|
|
|
|
bool retval = false;
|
|
|
|
int deleted_iters = 0;
|
|
|
|
int num_iters = 0;
|
2015-08-25 20:40:58 +00:00
|
|
|
|
|
|
|
const VersionStorageInfo* vstorage = sv_->current->storage_info();
|
|
|
|
const std::vector<FileMetaData*>& l0 = vstorage->LevelFiles(0);
|
2015-12-15 23:26:20 +00:00
|
|
|
for (size_t i = 0; i < l0.size(); ++i) {
|
2015-08-25 20:40:58 +00:00
|
|
|
if (!l0_iters_[i]) {
|
2015-09-04 21:28:45 +00:00
|
|
|
retval = true;
|
|
|
|
deleted_iters++;
|
|
|
|
} else {
|
|
|
|
num_iters++;
|
2015-08-25 20:40:58 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
for (int32_t level = 1; level < vstorage->num_levels(); ++level) {
|
|
|
|
if ((level_iters_[level - 1] == nullptr) &&
|
|
|
|
(!vstorage->LevelFiles(level).empty())) {
|
2015-09-04 21:28:45 +00:00
|
|
|
retval = true;
|
|
|
|
deleted_iters++;
|
|
|
|
} else if (!vstorage->LevelFiles(level).empty()) {
|
|
|
|
num_iters++;
|
2015-08-25 20:40:58 +00:00
|
|
|
}
|
|
|
|
}
|
2015-09-04 21:28:45 +00:00
|
|
|
if ((!retval) && num_iters <= 1) {
|
|
|
|
retval = true;
|
|
|
|
}
|
|
|
|
if (pdeleted_iters) {
|
|
|
|
*pdeleted_iters = deleted_iters;
|
|
|
|
}
|
|
|
|
if (pnum_iters) {
|
|
|
|
*pnum_iters = num_iters;
|
|
|
|
}
|
|
|
|
return retval;
|
2015-08-25 20:40:58 +00:00
|
|
|
}
|
2015-09-04 21:28:45 +00:00
|
|
|
|
2014-05-30 21:31:55 +00:00
|
|
|
uint32_t ForwardIterator::FindFileInRange(
|
|
|
|
const std::vector<FileMetaData*>& files, const Slice& internal_key,
|
|
|
|
uint32_t left, uint32_t right) {
|
2021-03-10 04:47:50 +00:00
|
|
|
auto cmp = [&](const FileMetaData* f, const Slice& k) -> bool {
|
2018-09-27 17:33:04 +00:00
|
|
|
return cfd_->internal_comparator().InternalKeyComparator::Compare(
|
2022-11-02 21:34:24 +00:00
|
|
|
f->largest.Encode(), k) < 0;
|
2018-09-27 17:33:04 +00:00
|
|
|
};
|
2022-11-02 21:34:24 +00:00
|
|
|
const auto& b = files.begin();
|
|
|
|
return static_cast<uint32_t>(
|
|
|
|
std::lower_bound(b + left, b + right, internal_key, cmp) - b);
|
2014-05-30 21:31:55 +00:00
|
|
|
}
|
|
|
|
|
2016-08-12 02:10:16 +00:00
|
|
|
void ForwardIterator::DeleteIterator(InternalIterator* iter, bool is_arena) {
|
|
|
|
if (iter == nullptr) {
|
|
|
|
return;
|
|
|
|
}
|
|
|
|
|
|
|
|
if (pinned_iters_mgr_ && pinned_iters_mgr_->PinningEnabled()) {
|
|
|
|
pinned_iters_mgr_->PinIterator(iter, is_arena);
|
|
|
|
} else {
|
|
|
|
if (is_arena) {
|
|
|
|
iter->~InternalIterator();
|
|
|
|
} else {
|
|
|
|
delete iter;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2020-02-20 20:07:53 +00:00
|
|
|
} // namespace ROCKSDB_NAMESPACE
|