From 1dbe59f0e82988e2c68f4e1d6f828d9e4ac86b83 Mon Sep 17 00:00:00 2001 From: Xingbo Wang Date: Tue, 10 Nov 2026 22:18:39 -0800 Subject: [PATCH] Fix prefix_same_as_start iterator correctness gaps (#15606) Summary: Pull Request resolved: https://github.com/meta/rocksdb/pull/15606 Why the change is required: The crash test found prefix_same_as_start iterators returning wrong keys in two ways, both caused by child iterators using prefix filters (SST prefix filter, memtable prefix bloom, LevelIterator prefix exhaustion) for a key in a different prefix than the one the caller iterates: 1. A scan with no seek prefix to bound it: SeekToFirst() / SeekToLast() (with a bound they take the prefix from the bound), or Seek() / SeekForPrev() with a target outside the prefix extractor's domain. With a range tombstone that crosses a prefix boundary, MergingIterator reseeks older children to the tombstone boundary key, a prefix filter can drop a child, and the scan can skip live keys. Read-path tombstone conversion (min_tombstones_for_range_conversion, EXPERIMENTAL) trusts these scans, so a skipped live key could be covered by a converted tombstone and deleted for good. The same holds for implicit prefix seek (default ReadOptions). Also, SeekToLast() with iterate_upper_bound becomes SeekForPrev(upper_bound), which takes its prefix from an exclusive bound the iterator can never return: keys "abcd1".."abcd9" with upper bound "abce" return nothing. 2. With prefix_same_as_start and total_order_seek both set, a direction change (Next() after Prev()) can skip keys inside the seek prefix. Under DBOptions::prefix_seek_opt_in_only, total_order_seek is forced on, so every prefix_same_as_start iterator is exposed. Previous behavior: 1. SeekToFirst()/SeekToLast() and out-of-domain seeks in prefix seek mode were never promised to match total order (the option docs call keys outside the seek prefix undefined, and the Prefix-Seek wiki says these calls are not well supported), but they ran, could return wrong results, and could create read-path range tombstones. 2. Since prefix_seek_opt_in_only was added (9.7.0), prefix_same_as_start and auto_prefix_mode enable prefix filtering even with total_order_seek=true, so for iterators total_order_seek only turns off the implicit prefix seek of default ReadOptions. This was stated in that release note, but not in options.h, where total_order_seek still read as "always total order". DBIter still treated total_order_seek=true as "the inner iterator is total order". So DBIter::ReverseToForward() skipped its reseek to the current key, and MergingIterator::SwitchToForward() reseeked the other children to the previous key, which can have another prefix. A child without that prefix was dropped by its prefix filter, losing its keys in the seek prefix. New behavior: 1. With DBOptions::prefix_seek_opt_in_only, a prefix_same_as_start scan must take its prefix from an in-domain seek key. SeekToFirst() and SeekToLast(), with or without bounds, leave the iterator !Valid() with Status::NotSupported, and Seek()/SeekForPrev() with an out-of-domain target leave it !Valid() with Status::InvalidArgument. Callers Seek() or SeekForPrev() to a key in the wanted prefix instead. Without prefix_seek_opt_in_only, iteration results are unchanged, but an iterator in prefix seek mode (prefix_same_as_start or implicit prefix seek) no longer creates read-path range tombstones from a scan started by SeekToFirst() without iterate_lower_bound, SeekToLast() without iterate_upper_bound, or an out-of-domain seek target. 2. In DBIter, prefix_same_as_start takes precedence over total_order_seek and auto_prefix_mode, so DBIter reseeks on a direction change. A prefix_same_as_start iterator returns the same keys with or without total_order_seek, and so with or without prefix_seek_opt_in_only for Seek()-started scans. Total order and auto_prefix_mode iteration results are not affected. Implementation: - GetIteratorPrefixMode() in options_helper.h decides how an iterator uses the prefix extractor (kTotalOrder, kAuto, kPrefix), with the order prefix_same_as_start > auto_prefix_mode > total_order_seek > implicit prefix seek. DBIter uses it for its total-order assumption; that is the direction-change fix. MemTable, PlainTable, the block-based table check_filter, LevelIterator files_filterable_on_seek_ and db_stress VerifyIterator use it where it gives the same result as before. The MergingIterator prefix flag, LevelIterator prefix exhaustion, the SST auto_prefix_mode upper bound check and the hash index keep their old conditions, so their behavior is unchanged. - DBIter: under prefix_seek_opt_in_only, prefix_same_as_start SeekToFirst() and SeekToLast() return NotSupported before redirecting to a bound, and a new CheckSeekTargetPrefix() at the start of Seek()/SeekForPrev() returns InvalidArgument for an out-of-domain target. Otherwise DBIter remembers when the current scan uses prefix seek but has no prefix to bound it (unbounded SeekToFirst()/SeekToLast(), or out-of-domain target), and MaybeInsertRangeTombstone(), the only place that writes a converted tombstone, does nothing then. - options.h: ReadOptions::total_order_seek now starts with its iterator meaning (turns off the implicit prefix seek; prefix_same_as_start and auto_prefix_mode take precedence; forced on by prefix_seek_opt_in_only). auto_prefix_mode notes that prefix_same_as_start takes precedence. prefix_seek_opt_in_only and prefix_same_as_start document the NotSupported / InvalidArgument cases and the scans that are not well supported. iterate_lower_bound allows a bound with another prefix under prefix_same_as_start. - Release notes: a behavior change for NotSupported / InvalidArgument and the conversion change, and a bug fix for the direction change. - db_stress: new prefix_seek_opt_in_only flag, randomized by db_crashtest.py. TestIterate sometimes sets total_order_seek together with prefix_same_as_start, and under prefix_seek_opt_in_only checks that SeekToFirst()/SeekToLast() return NotSupported. The prefix_same_as_start SeekToFirst() verification is removed: it checks a known limitation, and range tombstones left by earlier crash-test runs would make it fail. The bug it covered keeps its unit tests in file/prefetch_test.cc. Safety, compatibility, and limitations: - No API or format change. Users with prefix_seek_opt_in_only who call SeekToFirst()/SeekToLast() on a prefix_same_as_start iterator, or seek it to an out-of-domain key, now get NotSupported / InvalidArgument; they can Seek()/SeekForPrev() to a key in the wanted prefix, or use total order or auto_prefix_mode. - prefix_same_as_start with total_order_seek or prefix_seek_opt_in_only now reseeks on a direction change, the cost plain prefix_same_as_start already pays. - Without prefix_seek_opt_in_only, those scans can still skip live keys when range tombstones cross prefix boundaries, and unbounded legacy prefix-mode scans have other known holes. These stay documented limitations; only read-path conversion is turned off for them. - Using the prefix optimizations of plain prefix_same_as_start in the other layers when total_order_seek or auto_prefix_mode is also set is left for a follow-up. Differential Revision: D129901533 --- db/db_iter.cc | 51 +++++- db/db_iter.h | 19 ++ db/db_iterator_test.cc | 152 +++++++++++++++- db/db_range_del_test.cc | 172 ++++++++++++++++++ db/memtable.cc | 7 +- db/version_set.cc | 7 +- db_stress_tool/db_stress_common.h | 1 + db_stress_tool/db_stress_gflags.cc | 4 + db_stress_tool/db_stress_stat.h | 10 - db_stress_tool/db_stress_test_base.cc | 91 +++++---- include/rocksdb/options.h | 35 +++- options/options_helper.h | 35 ++++ table/block_based/block_based_table_reader.cc | 6 +- table/plain/plain_table_reader.cc | 5 +- tools/db_crashtest.py | 1 + ...x_same_as_start_unbounded_seek_to_first.md | 1 + ...fix_same_as_start_with_total_order_seek.md | 1 + 17 files changed, 513 insertions(+), 85 deletions(-) create mode 100644 unreleased_history/behavior_changes/prefix_same_as_start_unbounded_seek_to_first.md create mode 100644 unreleased_history/bug_fixes/prefix_same_as_start_with_total_order_seek.md diff --git a/db/db_iter.cc b/db/db_iter.cc index d6b201389a9..47ba1b14906 100644 --- a/db/db_iter.cc +++ b/db/db_iter.cc @@ -114,10 +114,13 @@ DBIter::DBIter(Env* _env, const ReadOptions& read_options, is_key_seqnum_zero_(false), prefix_same_as_start_( prefix_extractor_ ? read_options.prefix_same_as_start : false), + strict_prefix_same_as_start_(prefix_same_as_start_ && + ioptions.prefix_seek_opt_in_only), + unbounded_prefix_scan_(false), pin_thru_lifetime_(read_options.pin_data), - expect_total_order_inner_iter_(prefix_extractor_ == nullptr || - read_options.total_order_seek || - read_options.auto_prefix_mode), + expect_total_order_inner_iter_( + GetIteratorPrefixMode(read_options, prefix_extractor_ != nullptr) != + IteratorPrefixMode::kPrefix), // Read-path range conversion assumes the scan can observe all interior // live keys. table_filter can hide whole SSTs, read tiers other than // kReadAllTier can skip SSTs or uncached blocks, and timestamp filtering @@ -1779,7 +1782,8 @@ void DBIter::FlushPendingTombstoneRun(const Slice& end_key, } void DBIter::MaybeInsertRangeTombstone(const Slice& end_key) { - if (contiguous_tombstone_count_ < min_tombstones_for_range_conversion_) { + if (contiguous_tombstone_count_ < min_tombstones_for_range_conversion_ || + unbounded_prefix_scan_) { return; } @@ -2031,6 +2035,9 @@ void DBIter::Seek(const Slice& target) { PERF_COUNTER_ADD(iter_seek_count, 1); PERF_CPU_TIMER_GUARD(iter_seek_cpu_nanos, clock_); StopWatch sw(clock_, statistics_, DB_SEEK); + if (!CheckSeekTargetPrefix(target)) { + return; + } if (scan_opts_.has_value()) { if (scan_opts_->reverse) { @@ -2147,6 +2154,9 @@ void DBIter::SeekForPrev(const Slice& target) { PERF_COUNTER_ADD(iter_seek_count, 1); PERF_CPU_TIMER_GUARD(iter_seek_cpu_nanos, clock_); StopWatch sw(clock_, statistics_, DB_SEEK); + if (!CheckSeekTargetPrefix(target)) { + return; + } if (scan_opts_.has_value()) { if (!scan_opts_->reverse) { @@ -2267,11 +2277,35 @@ void DBIter::SeekForPrev(const Slice& target) { } } +bool DBIter::CheckSeekTargetPrefix(const Slice& target) { + unbounded_prefix_scan_ = false; + if (expect_total_order_inner_iter_ || prefix_extractor_->InDomain(target)) { + return true; + } + if (strict_prefix_same_as_start_) { + valid_ = false; + status_ = Status::InvalidArgument( + "Seek target outside the prefix extractor's domain is not supported " + "with prefix_same_as_start under prefix_seek_opt_in_only"); + return false; + } + unbounded_prefix_scan_ = true; + return true; +} + void DBIter::SeekToFirst() { + if (strict_prefix_same_as_start_) { + valid_ = false; + status_ = Status::NotSupported( + "SeekToFirst() is not supported with prefix_same_as_start under " + "prefix_seek_opt_in_only; Seek() to a key in the wanted prefix"); + return; + } if (iterate_lower_bound_ != nullptr) { Seek(*iterate_lower_bound_); return; } + unbounded_prefix_scan_ = !expect_total_order_inner_iter_; PERF_COUNTER_ADD(iter_seek_count, 1); PERF_CPU_TIMER_GUARD(iter_seek_cpu_nanos, clock_); // Don't use iter_::Seek() if we set a prefix extractor @@ -2322,6 +2356,14 @@ void DBIter::SeekToFirst() { } void DBIter::SeekToLast() { + if (strict_prefix_same_as_start_) { + valid_ = false; + status_ = Status::NotSupported( + "SeekToLast() is not supported with prefix_same_as_start under " + "prefix_seek_opt_in_only; SeekForPrev() to a key in the wanted " + "prefix"); + return; + } if (iterate_upper_bound_ != nullptr) { // Seek to last key strictly less than ReadOptions.iterate_upper_bound. SeekForPrev(*iterate_upper_bound_); @@ -2336,6 +2378,7 @@ void DBIter::SeekToLast() { #endif return; } + unbounded_prefix_scan_ = !expect_total_order_inner_iter_; PERF_COUNTER_ADD(iter_seek_count, 1); PERF_CPU_TIMER_GUARD(iter_seek_cpu_nanos, clock_); diff --git a/db/db_iter.h b/db/db_iter.h index a0937b20ac9..a6bee5ab7b0 100644 --- a/db/db_iter.h +++ b/db/db_iter.h @@ -612,6 +612,12 @@ class DBIter final : public Iterator { prefix_extractor_->InDomain(target); } + // Called at the start of Seek() and SeekForPrev(). Returns false, with + // status_ set, if target is outside the prefix extractor's domain under + // strict_prefix_same_as_start_. Otherwise sets unbounded_prefix_scan_ when + // target gives no prefix to bound a prefix seek. + bool CheckSeekTargetPrefix(const Slice& target); + void ResetSeekState() { ReleaseTempPinnedData(); ResetBlobData(); @@ -738,6 +744,19 @@ class DBIter final : public Iterator { // user key. bool is_key_seqnum_zero_; const bool prefix_same_as_start_; + // prefix_same_as_start_ under DBOptions::prefix_seek_opt_in_only. Every scan + // must take its prefix from an in-domain seek key: SeekToFirst() and + // SeekToLast() set NotSupported, and Seek() and SeekForPrev() with a target + // outside the prefix extractor's domain set InvalidArgument. + const bool strict_prefix_same_as_start_; + // True while the current scan uses prefix seek in the child iterators but + // has no prefix to bound it: after SeekToFirst() without + // iterate_lower_bound, SeekToLast() without iterate_upper_bound, or a + // Seek()/SeekForPrev() target outside the prefix extractor's domain. Prefix + // seek in the child iterators can then skip live keys, so read-path range + // tombstone conversion is disabled so that a skipped live key cannot be + // covered by a new tombstone. + bool unbounded_prefix_scan_; // Means that we will pin all data blocks we read as long the Iterator // is not deleted, will be true if ReadOptions::pin_data is true const bool pin_thru_lifetime_; diff --git a/db/db_iterator_test.cc b/db/db_iterator_test.cc index f3019dc41aa..7a9b142a1fd 100644 --- a/db/db_iterator_test.cc +++ b/db/db_iterator_test.cc @@ -87,6 +87,147 @@ class DBIteratorBaseTest : public DBTestBase { }; #ifdef OS_LINUX +TEST_F(DBIteratorBaseTest, PrefixSameAsStartDirectionChange) { + // prefix_same_as_start uses prefix seek even with total_order_seek=true (as + // forced by prefix_seek_opt_in_only) or auto_prefix_mode. On Next() after + // Prev(), the inner iterator stands on a key with another prefix (bbbb1). + // DBIter must reseek it to the current key; moving it without a reseek + // seeks the other children to bbbb1, where a prefix filter drops the child + // holding kkkk1 and kkkk3. + struct ReadCase { + const char* name; + bool opt_in; + bool total_order_seek; + bool auto_prefix_mode; + }; + const std::vector read_cases = { + {"prefix_same_as_start", false, false, false}, + {"+total_order_seek", false, true, false}, + {"+auto_prefix_mode", false, false, true}, + {"+prefix_seek_opt_in_only", true, false, false}, + }; + for (bool memtable_bloom : {false, true}) { + for (const ReadCase& rc : read_cases) { + SCOPED_TRACE(std::string(rc.name) + + (memtable_bloom ? " memtable_bloom" : " sst_filter")); + Options options = CurrentOptions(); + options.disable_auto_compactions = true; + options.prefix_extractor.reset(NewFixedPrefixTransform(4)); + options.prefix_seek_opt_in_only = rc.opt_in; + BlockBasedTableOptions table_options; + if (memtable_bloom) { + options.memtable_prefix_bloom_size_ratio = 0.1; + } else { + table_options.filter_policy.reset(NewBloomFilterPolicy(10, false)); + table_options.whole_key_filtering = false; + } + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + DestroyAndReopen(options); + // The child that gets filtered has prefix kkkk only: the SST for the + // SST filter case, the memtable for the memtable bloom case. + const std::vector kkkk_only = {"kkkk1", "kkkk3"}; + const std::vector mixed = {"bbbb1", "kkkk2"}; + for (const std::string& key : memtable_bloom ? mixed : kkkk_only) { + ASSERT_OK(Put(key, "v")); + } + ASSERT_OK(Flush()); + for (const std::string& key : memtable_bloom ? kkkk_only : mixed) { + ASSERT_OK(Put(key, "v")); + } + + ReadOptions ro; + ro.prefix_same_as_start = true; + ro.total_order_seek = rc.total_order_seek; + ro.auto_prefix_mode = rc.auto_prefix_mode; + std::unique_ptr iter(db_->NewIterator(ro)); + iter->Seek("kkkk1"); + std::vector keys; + // Seek, Next, Prev, then Next until the end of the prefix. + for (int step = 0; iter->Valid(); ++step) { + keys.push_back(iter->key().ToString()); + if (step == 1) { + iter->Prev(); + } else { + iter->Next(); + } + } + ASSERT_OK(iter->status()); + const std::vector expected = {"kkkk1", "kkkk2", "kkkk1", + "kkkk2", "kkkk3"}; + EXPECT_EQ(expected, keys); + } + } +} + +TEST_F(DBIteratorBaseTest, PrefixSameAsStartWithExplicitBounds) { + // prefix_same_as_start implies a bound at the end of the seek prefix. With + // or without auto_prefix_mode, the iterator returns only keys within both + // that implied bound and the explicit iterate_lower_bound / + // iterate_upper_bound, whichever is narrower. + Options options = CurrentOptions(); + options.disable_auto_compactions = true; + options.prefix_extractor.reset(NewFixedPrefixTransform(4)); + BlockBasedTableOptions table_options; + table_options.filter_policy.reset(NewBloomFilterPolicy(10, false)); + table_options.whole_key_filtering = false; + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + DestroyAndReopen(options); + for (const char* key : {"kkkj9", "kkkk1", "kkkk5"}) { + ASSERT_OK(Put(key, "v")); + } + ASSERT_OK(Flush()); + for (const char* key : {"kkkk3", "kkkl1"}) { + ASSERT_OK(Put(key, "v")); + } + + struct BoundCase { + const char* name; + const char* bound; + std::vector expected; + }; + const std::vector forward_cases = { + {"upper bound inside prefix", "kkkk4", {"kkkk1", "kkkk3"}}, + {"upper bound past prefix", "kkkm", {"kkkk1", "kkkk3", "kkkk5"}}, + }; + const std::vector backward_cases = { + {"lower bound inside prefix", "kkkk2", {"kkkk5", "kkkk3"}}, + {"lower bound before prefix", "kkkj", {"kkkk5", "kkkk3", "kkkk1"}}, + }; + for (bool auto_prefix_mode : {false, true}) { + ReadOptions ro; + ro.prefix_same_as_start = true; + ro.auto_prefix_mode = auto_prefix_mode; + for (const BoundCase& bc : forward_cases) { + SCOPED_TRACE(std::string(bc.name) + + (auto_prefix_mode ? " auto_prefix_mode" : "")); + Slice upper_bound(bc.bound); + ro.iterate_upper_bound = &upper_bound; + std::unique_ptr iter(db_->NewIterator(ro)); + std::vector keys; + for (iter->Seek("kkkk1"); iter->Valid(); iter->Next()) { + keys.push_back(iter->key().ToString()); + } + ASSERT_OK(iter->status()); + EXPECT_EQ(bc.expected, keys); + } + ro.iterate_upper_bound = nullptr; + for (const BoundCase& bc : backward_cases) { + SCOPED_TRACE(std::string(bc.name) + + (auto_prefix_mode ? " auto_prefix_mode" : "")); + Slice lower_bound(bc.bound); + ro.iterate_lower_bound = &lower_bound; + std::unique_ptr iter(db_->NewIterator(ro)); + std::vector keys; + for (iter->SeekForPrev("kkkk5"); iter->Valid(); iter->Prev()) { + keys.push_back(iter->key().ToString()); + } + ASSERT_OK(iter->status()); + EXPECT_EQ(bc.expected, keys); + } + ro.iterate_lower_bound = nullptr; + } +} + TEST_F(DBIteratorBaseTest, PrevChecksQueryAbort) { ASSERT_OK(Put("a", "va")); ASSERT_OK(Put("b", "vb")); @@ -8101,10 +8242,10 @@ TEST_P(ReadPathRangeTombstoneTest, TEST_P(ReadPathRangeTombstoneTest, PrefixFilterOutOfDomainSeek) { // With an out-of-domain seek target, prefix_same_as_start cannot establish - // a seek-prefix bound. The iterator therefore behaves like an unrestricted - // scan while still avoiding Transform() on the short seek target, so the - // valid run [bbbb, bbdd) should be converted even though it spans multiple - // distinct prefixes. + // a seek-prefix bound. The iterator scans past the seek prefix while the + // child iterators still use prefix seek, which can skip live keys, so the + // run [bbbb, bbdd) must not be converted. The scan must still avoid + // Transform() on the short seek target. Options options = CurrentOptions(); options.min_tombstones_for_range_conversion = 2; options.statistics = CreateDBStatistics(); @@ -8144,8 +8285,7 @@ TEST_P(ReadPathRangeTombstoneTest, PrefixFilterOutOfDomainSeek) { } } ASSERT_OK(it->status()); - ASSERT_EQ(attempted_insert_ranges_.size(), 1u); - AssertRange(0, "bbbb", "bbdd"); + ASSERT_TRUE(attempted_insert_ranges_.empty()); } TEST_P(ReadPathRangeTombstoneTest, TableFilterNotAllowed) { diff --git a/db/db_range_del_test.cc b/db/db_range_del_test.cc index 020442e0965..1c7e088c739 100644 --- a/db/db_range_del_test.cc +++ b/db/db_range_del_test.cc @@ -3165,6 +3165,178 @@ TEST_F(DBRangeDelTest, PrefixSentinelKey) { delete iter; } +namespace { +Options PrefixFilterOptions(const Options& base) { + Options options = base; + options.disable_auto_compactions = true; + options.prefix_extractor.reset(NewFixedPrefixTransform(4)); + BlockBasedTableOptions table_options; + table_options.filter_policy.reset(NewBloomFilterPolicy(10, false)); + table_options.whole_key_filtering = false; + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + return options; +} + +// Scans the whole DB from SeekToFirst() (or SeekToLast() if !forward). +std::vector ScanAllKeys(DB* db, const ReadOptions& ro, + bool forward) { + std::vector keys; + std::unique_ptr iter(db->NewIterator(ro)); + if (forward) { + for (iter->SeekToFirst(); iter->Valid(); iter->Next()) { + keys.push_back(iter->key().ToString()); + } + } else { + for (iter->SeekToLast(); iter->Valid(); iter->Prev()) { + keys.push_back(iter->key().ToString()); + } + } + EXPECT_OK(iter->status()); + return keys; +} +} // namespace + +// Older data with prefixes aaaa, kkkk and xxxx only; newer tombstones whose +// boundary keys have prefixes cccc and pppp, which the older data lacks. +class DBRangeDelCrossPrefixTest : public DBRangeDelTest { + protected: + void Populate(const Options& options, int older_level) { + DestroyAndReopen(options); + ASSERT_OK(Put("aaaa1", "v")); + ASSERT_OK(Put("kkkk1", "v")); + ASSERT_OK(Put("kkkk5", "v")); + ASSERT_OK(Put("xxxx1", "v")); + ASSERT_OK(Flush()); + if (older_level > 0) { + MoveFilesToLevel(older_level); + ASSERT_EQ(1, NumTableFilesAtLevel(older_level)); + } + ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), + "aaaa", "cccc2")); + ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), + "pppp", "zzzz")); + } + const std::vector kLiveKeys = {"kkkk1", "kkkk5"}; + const std::vector kLiveKeysReversed = {"kkkk5", "kkkk1"}; +}; + +TEST_F(DBRangeDelCrossPrefixTest, PrefixSameAsStartOptInRequiresSeekPrefix) { + // Under prefix_seek_opt_in_only, a prefix_same_as_start scan must take its + // prefix from an in-domain seek key, with or without bounds. With upper + // bound "kkkl", SeekToLast() would otherwise take prefix "kkkl" from the + // bound and return no keys. + Options options = PrefixFilterOptions(CurrentOptions()); + options.prefix_seek_opt_in_only = true; + Populate(options, /*older_level=*/1); + + const Slice lower_bound("kkkk"); + const Slice upper_bound("kkkl"); + for (bool with_bounds : {false, true}) { + SCOPED_TRACE(with_bounds ? "with bounds" : "without bounds"); + ReadOptions ro; + ro.prefix_same_as_start = true; + if (with_bounds) { + ro.iterate_lower_bound = &lower_bound; + ro.iterate_upper_bound = &upper_bound; + } + std::unique_ptr iter(db_->NewIterator(ro)); + iter->SeekToFirst(); + ASSERT_FALSE(iter->Valid()); + ASSERT_TRUE(iter->status().IsNotSupported()); + iter->SeekToLast(); + ASSERT_FALSE(iter->Valid()); + ASSERT_TRUE(iter->status().IsNotSupported()); + // "kkk" is shorter than the 4-byte prefix, so it is out of domain. + iter->Seek("kkk"); + ASSERT_FALSE(iter->Valid()); + ASSERT_TRUE(iter->status().IsInvalidArgument()); + iter->SeekForPrev("kkk"); + ASSERT_FALSE(iter->Valid()); + ASSERT_TRUE(iter->status().IsInvalidArgument()); + + // Seek() and SeekForPrev() to a key in the prefix still work. + std::vector keys; + for (iter->Seek("kkkk"); iter->Valid(); iter->Next()) { + keys.push_back(iter->key().ToString()); + } + ASSERT_OK(iter->status()); + ASSERT_EQ(kLiveKeys, keys); + keys.clear(); + for (iter->SeekForPrev("kkkk9"); iter->Valid(); iter->Prev()) { + keys.push_back(iter->key().ToString()); + } + ASSERT_OK(iter->status()); + ASSERT_EQ(kLiveKeysReversed, keys); + } + + // Total order and auto_prefix_mode iterators are not affected. + ReadOptions total_order; + ASSERT_EQ(kLiveKeys, ScanAllKeys(db_.get(), total_order, /*forward=*/true)); + ReadOptions auto_prefix; + auto_prefix.auto_prefix_mode = true; + ASSERT_EQ(kLiveKeys, ScanAllKeys(db_.get(), auto_prefix, /*forward=*/true)); +} + +TEST_F(DBRangeDelCrossPrefixTest, AutoPrefixModeFullScanCrossPrefixTombstone) { + ReadOptions auto_prefix; + auto_prefix.auto_prefix_mode = true; + for (int older_level : {0, 1}) { + SCOPED_TRACE("older_level=" + std::to_string(older_level)); + Populate(PrefixFilterOptions(CurrentOptions()), older_level); + ASSERT_EQ(kLiveKeys, ScanAllKeys(db_.get(), auto_prefix, /*forward=*/true)); + ASSERT_EQ(kLiveKeysReversed, + ScanAllKeys(db_.get(), auto_prefix, /*forward=*/false)); + } +} + +TEST_F(DBRangeDelTest, PrefixSeekUnboundedScanNoRangeConversion) { + // A prefix seek scan with no prefix to bound it (unbounded SeekToFirst(), or + // a seek target outside the prefix extractor's domain) can skip live keys, + // so it must not convert point deletes into a range tombstone. This holds + // for prefix_same_as_start and for implicit prefix seek (default + // ReadOptions). + Options options = PrefixFilterOptions(CurrentOptions()); + options.min_tombstones_for_range_conversion = 2; + options.statistics = CreateDBStatistics(); + DestroyAndReopen(options); + ASSERT_OK(Put("kkkk1", "v")); + ASSERT_OK(Flush()); + ASSERT_OK(Delete("aaaa1")); + ASSERT_OK(Delete("aaaa2")); + ASSERT_OK(Delete("aaaa3")); + + ReadOptions ro; + ro.prefix_same_as_start = true; + for (bool prefix_same_as_start : {true, false}) { + ReadOptions scan_ro; + scan_ro.prefix_same_as_start = prefix_same_as_start; + for (bool seek_to_first : {true, false}) { + SCOPED_TRACE(std::string(prefix_same_as_start ? "prefix_same_as_start" + : "implicit prefix seek") + + (seek_to_first ? " SeekToFirst" : " out-of-domain Seek")); + std::unique_ptr iter(db_->NewIterator(scan_ro)); + if (seek_to_first) { + iter->SeekToFirst(); + } else { + // "aaa" is shorter than the 4-byte prefix, so it is out of domain. + iter->Seek("aaa"); + } + ASSERT_OK(iter->status()); + ASSERT_EQ(0, options.statistics->getTickerCount( + READ_PATH_RANGE_TOMBSTONES_INSERTED)); + } + } + // A scan started by an in-domain Seek() still converts the same deletes. + { + std::unique_ptr iter(db_->NewIterator(ro)); + iter->Seek("aaaa"); + ASSERT_OK(iter->status()); + ASSERT_FALSE(iter->Valid()); + } + ASSERT_EQ(1, options.statistics->getTickerCount( + READ_PATH_RANGE_TOMBSTONES_INSERTED)); +} + TEST_F(DBRangeDelTest, RefreshMemtableIter) { Options options = CurrentOptions(); options.disable_auto_compactions = true; diff --git a/db/memtable.cc b/db/memtable.cc index 22abd49cd76..57db773b120 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -31,6 +31,7 @@ #include "memory/memory_usage.h" #include "monitoring/perf_context_imp.h" #include "monitoring/statistics_impl.h" +#include "options/options_helper.h" #include "port/lang.h" #include "port/port.h" #include "rocksdb/comparator.h" @@ -523,9 +524,9 @@ class MemTableIterator : public InternalIterator { // NOTE: checking extractor equivalence when not pointer // equivalent is arguably too expensive for memtable prefix_extractor_ == cf_prefix_extractor && - (read_options.prefix_same_as_start || - (!read_options.total_order_seek && - !read_options.auto_prefix_mode))) { + GetIteratorPrefixMode(read_options, + /*has_prefix_extractor=*/true) == + IteratorPrefixMode::kPrefix) { // Auto prefix mode is not implemented in memtable yet. assert(kind == kPointEntries); bloom_ = mem.bloom_filter_.get(); diff --git a/db/version_set.cc b/db/version_set.cc index d0d0962cb57..77dd330aeff 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1221,11 +1221,10 @@ class LevelIterator final : public InternalIterator { // prefix filter on Seek(). TableCache hands out no reference to an // ephemeral table reader, which a stand-in would need. files_filterable_on_seek_( - prefix_extractor_ != nullptr && !skip_filters_ && - !table_cache_open_options_.skip_filters && + !skip_filters_ && !table_cache_open_options_.skip_filters && !table_cache_open_options_.open_ephemeral_table_reader && - (!read_options.total_order_seek || read_options.auto_prefix_mode || - read_options.prefix_same_as_start)) { + GetIteratorPrefixMode(read_options, prefix_extractor_ != nullptr) != + IteratorPrefixMode::kTotalOrder) { // Empty level is not supported. assert(flevel_ != nullptr && flevel_->num_files > 0); if (range_tombstone_iter_ptr_) { diff --git a/db_stress_tool/db_stress_common.h b/db_stress_tool/db_stress_common.h index 43d4e6aa0ac..a5b0b6d4029 100644 --- a/db_stress_tool/db_stress_common.h +++ b/db_stress_tool/db_stress_common.h @@ -319,6 +319,7 @@ DECLARE_bool(write_dbid_to_manifest); DECLARE_bool(track_options_file_number_in_manifest); DECLARE_bool(optimize_manifest_for_recovery); DECLARE_bool(write_identity_file); +DECLARE_bool(prefix_seek_opt_in_only); DECLARE_bool(reuse_manifest_on_open); DECLARE_bool(avoid_flush_during_recovery); DECLARE_bool(enforce_write_buffer_manager_during_recovery); diff --git a/db_stress_tool/db_stress_gflags.cc b/db_stress_tool/db_stress_gflags.cc index 89202fcf992..40245af7f0c 100644 --- a/db_stress_tool/db_stress_gflags.cc +++ b/db_stress_tool/db_stress_gflags.cc @@ -1352,6 +1352,10 @@ DEFINE_bool(write_identity_file, ROCKSDB_NAMESPACE::Options().write_identity_file, "Write DB_ID to IDENTITY file"); +DEFINE_bool(prefix_seek_opt_in_only, + ROCKSDB_NAMESPACE::Options().prefix_seek_opt_in_only, + "Set DBOptions::prefix_seek_opt_in_only"); + DEFINE_bool(reuse_manifest_on_open, ROCKSDB_NAMESPACE::Options().reuse_manifest_on_open, "Reopen existing MANIFEST for append after recovery instead of " diff --git a/db_stress_tool/db_stress_stat.h b/db_stress_tool/db_stress_stat.h index b843d0c8f3d..0df641f8b7f 100644 --- a/db_stress_tool/db_stress_stat.h +++ b/db_stress_tool/db_stress_stat.h @@ -37,7 +37,6 @@ class Stats { long iterator_size_sums_; long founds_; long iterations_; - long seek_to_first_after_seek_; long range_deletions_; long covered_by_range_deletions_; long errors_; @@ -64,7 +63,6 @@ class Stats { iterator_size_sums_ = 0; founds_ = 0; iterations_ = 0; - seek_to_first_after_seek_ = 0; range_deletions_ = 0; covered_by_range_deletions_ = 0; errors_ = 0; @@ -89,7 +87,6 @@ class Stats { iterator_size_sums_ += other.iterator_size_sums_; founds_ += other.founds_; iterations_ += other.iterations_; - seek_to_first_after_seek_ += other.seek_to_first_after_seek_; range_deletions_ += other.range_deletions_; covered_by_range_deletions_ = other.covered_by_range_deletions_; errors_ += other.errors_; @@ -162,9 +159,6 @@ class Stats { void AddIterations(long n) { iterations_ += n; } - // SeekToFirst() after Seek() on one iterator under prefix_same_as_start. - void AddSeekToFirstAfterSeek(long n) { seek_to_first_after_seek_ += n; } - void AddDeletes(long n) { deletes_ += n; } void AddSingleDeletes(size_t n) { single_deletes_ += n; } @@ -211,10 +205,6 @@ class Stats { fprintf(stdout, "%-12s: Iterator size sum is %ld\n", "", iterator_size_sums_); fprintf(stdout, "%-12s: Iterated %ld times\n", "", iterations_); - fprintf(stdout, - "%-12s: SeekToFirst() after Seek() under " - "prefix_same_as_start %ld times\n", - "", seek_to_first_after_seek_); fprintf(stdout, "%-12s: Deleted %ld key-ranges\n", "", range_deletions_); fprintf(stdout, "%-12s: Range deletions covered %ld keys\n", "", covered_by_range_deletions_); diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 2cc21bd59be..a21fd100dac 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -3726,21 +3726,21 @@ Status StressTest::TestIterateImpl(ThreadState* thread, expect_total_order = true; } else if (thread->rand.OneIn(2)) { ro.prefix_same_as_start = true; + // prefix_same_as_start takes precedence over total_order_seek, which is + // also how every iterator looks under prefix_seek_opt_in_only. + ro.total_order_seek = thread->rand.OneIn(2); } - // Run SeekToFirst() after Seek() on the same table iterator under - // prefix_same_as_start, without bounds. DBIter converts SeekToFirst() with a - // lower bound to Seek(lower_bound). An upper bound inside the first prefix - // ends the scan before the verifier can tell a truncated scan from a complete - // one. prefix_hash returns nothing from a prefix SeekToFirst(). - const bool cover_seek_to_first_after_seek = - ro.prefix_same_as_start && FLAGS_memtablerep != "prefix_hash" && - thread->rand.OneIn(8); + // Under prefix_same_as_start, SeekToFirst() and SeekToLast() return + // NotSupported with prefix_seek_opt_in_only. Without it they are not well + // supported (they can skip live keys when range tombstones cross prefix + // boundaries), so they are not verified. + const bool expect_seek_to_first_last_not_supported = + ro.prefix_same_as_start && FLAGS_prefix_seek_opt_in_only; std::string upper_bound_str; Slice upper_bound; // Prefer no bound with no range query filtering; prefer bound with it - if (!cover_seek_to_first_after_seek && - (FLAGS_use_sqfc_for_range_queries ^ thread->rand.OneIn(16))) { + if (FLAGS_use_sqfc_for_range_queries ^ thread->rand.OneIn(16)) { // Note: upper_bound can be smaller than the seek key. const int64_t rand_upper_key = GenerateOneKey(thread, FLAGS_ops_per_thread); upper_bound_str = Key(rand_upper_key); @@ -3750,8 +3750,7 @@ Status StressTest::TestIterateImpl(ThreadState* thread, std::string lower_bound_str; Slice lower_bound; - if (!cover_seek_to_first_after_seek && - (FLAGS_use_sqfc_for_range_queries ^ thread->rand.OneIn(16))) { + if (FLAGS_use_sqfc_for_range_queries ^ thread->rand.OneIn(16)) { // Note: lower_bound can be greater than the seek key. const int64_t rand_lower_key = GenerateOneKey(thread, FLAGS_ops_per_thread); lower_bound_str = Key(rand_lower_key); @@ -3840,13 +3839,8 @@ Status StressTest::TestIterateImpl(ThreadState* thread, Slice key(key_str); - // DBIter converts SeekToFirst() with a lower bound to Seek(lower_bound), - // which VerifyIterator() does not handle. SeekToLast() is only verified - // under total order. const bool support_seek_to_first = - (expect_total_order && FLAGS_test_backward_scan) || - (ro.prefix_same_as_start && ro.iterate_lower_bound == nullptr && - FLAGS_memtablerep != "prefix_hash"); + expect_total_order && FLAGS_test_backward_scan; const bool support_seek_to_last = expect_total_order && FLAGS_test_backward_scan; const bool support_seek_for_prev = FLAGS_test_backward_scan; @@ -3863,31 +3857,30 @@ Status StressTest::TestIterateImpl(ThreadState* thread, op_logs += "Refresh "; } - LastIterateOp last_op; - // Under prefix_same_as_start a SeekToFirst() scan covers the prefix of the - // first key. - std::string verify_key_str; - Slice verify_key = key; - if (support_seek_to_first && - thread->rand.OneIn(cover_seek_to_first_after_seek ? 4 : 100)) { - if (cover_seek_to_first_after_seek) { - // If this Seek() read the first data block, the block would be cached - // and SeekToFirst() would not read the file. - const std::string pre_seek_key = - Key(GenerateOneKey(thread, FLAGS_ops_per_thread)); - iter->Seek(pre_seek_key); - cmp_iter->Seek(pre_seek_key); - op_logs += "PreS " + Slice(pre_seek_key).ToString(true) + " "; - thread->stats.AddSeekToFirstAfterSeek(1); + if (expect_seek_to_first_last_not_supported && thread->rand.OneIn(100)) { + const bool to_first = thread->rand.OneIn(2); + if (to_first) { + iter->SeekToFirst(); + } else { + iter->SeekToLast(); } + if (iter->Valid() || !iter->status().IsNotSupported()) { + thread->shared->SetVerificationFailure(); + fprintf(stderr, + "%s under prefix_same_as_start and prefix_seek_opt_in_only " + "returned %s, expected NotSupported\n", + to_first ? "SeekToFirst()" : "SeekToLast()", + iter->Valid() ? "a key" : iter->status().ToString().c_str()); + return Status::OK(); + } + } + + LastIterateOp last_op; + if (support_seek_to_first && thread->rand.OneIn(100)) { iter->SeekToFirst(); cmp_iter->SeekToFirst(); last_op = kLastOpSeekToFirst; op_logs += "STF "; - if (ro.prefix_same_as_start && cmp_iter->Valid()) { - verify_key_str = cmp_iter->key().ToString(); - verify_key = verify_key_str; - } } else if (support_seek_to_last && thread->rand.OneIn(100)) { iter->SeekToLast(); cmp_iter->SeekToLast(); @@ -3922,14 +3915,11 @@ Status StressTest::TestIterateImpl(ThreadState* thread, } VerifyIterator(thread, cmp_cfh, ro, iter.get(), cmp_iter.get(), last_op, - verify_key, rand_column_families, op_logs, verify_func, - &diverged); + key, rand_column_families, op_logs, verify_func, &diverged); - // Prev() resets the table iterator's readahead lookup state. const bool no_reverse = (FLAGS_memtablerep == "prefix_hash" && !expect_total_order) || - !FLAGS_test_backward_scan || - (cover_seek_to_first_after_seek && last_op == kLastOpSeekToFirst); + !FLAGS_test_backward_scan; for (uint64_t i = 0; i < FLAGS_num_iterations && iter->Valid(); ++i) { if (no_reverse || thread->rand.OneIn(2)) { iter->Next(); @@ -3966,7 +3956,7 @@ Status StressTest::TestIterateImpl(ThreadState* thread, } VerifyIterator(thread, cmp_cfh, ro, iter.get(), cmp_iter.get(), last_op, - verify_key, rand_column_families, op_logs, verify_func, + key, rand_column_families, op_logs, verify_func, &diverged); } @@ -4168,8 +4158,11 @@ bool StressTest::VerifyIterator( return false; } - if (!ro.total_order_seek && options_.prefix_extractor != nullptr && - ro.iterate_lower_bound != nullptr) { + // prefix_same_as_start takes precedence over total_order_seek. + const bool prefix_iteration = + GetIteratorPrefixMode(ro, options_.prefix_extractor != nullptr) == + IteratorPrefixMode::kPrefix; + if (prefix_iteration && ro.iterate_lower_bound != nullptr) { const SliceTransform* prefix_extractor = options_.prefix_extractor.get(); if (!prefix_extractor->InDomain(verify_key) || !prefix_extractor->InDomain(*ro.iterate_lower_bound) || @@ -4183,9 +4176,8 @@ bool StressTest::VerifyIterator( } } - const SliceTransform* pe = (ro.total_order_seek || ro.auto_prefix_mode) - ? nullptr - : options_.prefix_extractor.get(); + const SliceTransform* pe = + prefix_iteration ? options_.prefix_extractor.get() : nullptr; const Comparator* cmp = options_.comparator; std::ostringstream read_opt_oss; read_opt_oss << "pin_data: " << ro.pin_data @@ -7203,6 +7195,7 @@ void InitializeOptionsFromFlags( FLAGS_track_options_file_number_in_manifest; options.optimize_manifest_for_recovery = FLAGS_optimize_manifest_for_recovery; options.write_identity_file = FLAGS_write_identity_file; + options.prefix_seek_opt_in_only = FLAGS_prefix_seek_opt_in_only; options.reuse_manifest_on_open = FLAGS_reuse_manifest_on_open; options.avoid_flush_during_recovery = FLAGS_avoid_flush_during_recovery; options.enforce_write_buffer_manager_during_recovery = diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 91440512135..eab51ea1e39 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -1707,6 +1707,12 @@ struct DBOptions { // When set to true, it is as if every iterator is created with // total_order_seek=true and only auto_prefix_mode=true and // prefix_same_as_start=true can take advantage of prefix seek optimizations. + // A prefix_same_as_start iterator still uses prefix seek, although + // total_order_seek is set (see ReadOptions::total_order_seek). + // Also, a prefix_same_as_start iterator returns Status::NotSupported from + // SeekToFirst() and SeekToLast(), and Status::InvalidArgument from Seek() + // and SeekForPrev() with a target outside the prefix extractor's domain + // (see ReadOptions::prefix_same_as_start). bool prefix_seek_opt_in_only = false; // The number of bytes to prefetch when reading the DB manifest and WAL files @@ -2589,7 +2595,9 @@ struct ReadOptions { // // If prefix_extractor is not null, the Seek target and `iterate_lower_bound` // need to have the same prefix. This is because ordering is not guaranteed - // outside of prefix domain. + // outside of prefix domain. With prefix_same_as_start, `iterate_lower_bound` + // may have another prefix: the iterator stops at whichever of the bound and + // the start of the seek prefix it reaches first. // // In case of user_defined timestamp, if enabled, iterate_lower_bound should // point to key without timestamp part. @@ -2621,9 +2629,16 @@ struct ReadOptions { // that were inserted into the database after the creation of the iterator. bool tailing = false; - // Enable a total order seek regardless of index format (e.g. hash index) - // used in the table. Some table format (e.g. plain table) may not support - // this option. + // For iterators: turns off the implicit prefix seek that iterators with + // default ReadOptions use when a prefix_extractor is set, so they return + // keys in total order. prefix_same_as_start and auto_prefix_mode are + // explicit requests and take precedence: with prefix_same_as_start (even + // with total_order_seek=true) the iterator uses prefix seek and returns only + // keys with the seek key's prefix, when the seek key is in the prefix + // extractor's domain; with auto_prefix_mode, results are as in total + // order. DBOptions::prefix_seek_opt_in_only forces this option on. + // Also forces a total order index seek regardless of index format (e.g. + // hash index). Some table formats (e.g. plain table) may not support this. // If true when calling Get(), we also skip prefix bloom when reading from // block based table, which only affects Get() performance. bool total_order_seek = false; @@ -2631,6 +2646,7 @@ struct ReadOptions { // When true, by default use total_order_seek = true, and RocksDB can // selectively enable prefix seek mode if won't generate a different result // from total_order_seek, based on seek key, and iterator upper bound. + // prefix_same_as_start takes precedence over auto_prefix_mode. // BUG: Using Comparator::IsSameLengthImmediateSuccessor and // SliceTransform::FullLengthEnabled to enable prefix mode in cases where // prefix of upper bound differs from prefix of seek key has a flaw. @@ -2649,6 +2665,17 @@ struct ReadOptions { // prefix_extractor, which is mutable. When SST files have been built with // the same prefix extractor, prefix filtering optimizations will be used // for both Seek and SeekForPrev. + // + // SeekToFirst() and SeekToLast() have no seek key to take the prefix from + // (with a bound, the prefix comes from the bound), and neither does a Seek() + // or SeekForPrev() target outside the prefix extractor's domain. Such scans + // are not well supported with prefix seek: they can skip keys, for example + // when range tombstones cross prefix boundaries. Use total order iteration + // or auto_prefix_mode, instead of prefix_same_as_start, for such scans. With + // DBOptions::prefix_seek_opt_in_only, SeekToFirst() and SeekToLast() leave + // the iterator !Valid() with status NotSupported, and Seek() and + // SeekForPrev() with an out-of-domain target leave it !Valid() with status + // InvalidArgument. bool prefix_same_as_start = false; // Keep the blocks loaded by the iterator pinned in memory as long as the diff --git a/options/options_helper.h b/options/options_helper.h index bc840648492..e50c118f8eb 100644 --- a/options/options_helper.h +++ b/options/options_helper.h @@ -46,6 +46,41 @@ inline bool HasTableFilter(const ReadOptions& read_options) { return read_options.table_filter != nullptr && *read_options.table_filter; } +// How an iterator created with `read_options` uses the prefix extractor. Every +// iterator layer must decide with this function, so that they all agree. +enum class IteratorPrefixMode { + // No prefix seek. The iterator returns all keys in order. + kTotalOrder, + // auto_prefix_mode: results as in total order. Prefix filters are only used + // where iterate_upper_bound shows that a seek stays within one prefix. + kAuto, + // Prefix seek. Child iterators may skip keys outside the seek key's prefix + // (prefix filters, prefix indexes), so only keys within that prefix are + // reliable. + kPrefix, +}; + +// prefix_same_as_start and auto_prefix_mode are explicit requests and take +// precedence over total_order_seek. total_order_seek only turns off the +// implicit prefix seek of iterators with default ReadOptions (and is forced on +// by DBOptions::prefix_seek_opt_in_only). +inline IteratorPrefixMode GetIteratorPrefixMode(const ReadOptions& read_options, + bool has_prefix_extractor) { + if (!has_prefix_extractor) { + return IteratorPrefixMode::kTotalOrder; + } + if (read_options.prefix_same_as_start) { + return IteratorPrefixMode::kPrefix; + } + if (read_options.auto_prefix_mode) { + return IteratorPrefixMode::kAuto; + } + if (read_options.total_order_seek) { + return IteratorPrefixMode::kTotalOrder; + } + return IteratorPrefixMode::kPrefix; +} + // Checks that the combination of DBOptions and ColumnFamilyOptions are valid Status ValidateOptions(const DBOptions& db_opts, const ColumnFamilyOptions& cf_opts); diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 1adbf7e4a56..5802bbc5462 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -35,6 +35,7 @@ #include "logging/logging.h" #include "memory/memory_allocator_impl.h" #include "monitoring/perf_context_imp.h" +#include "options/options_helper.h" #include "parsed_full_filter_block.h" #include "port/lang.h" #include "rocksdb/cache.h" @@ -2808,9 +2809,8 @@ InternalIterator* BlockBasedTable::NewIteratorImpl( read_options.auto_prefix_mode || PrefixExtractorChanged(prefix_extractor); bool check_filter = !skip_filters && - (!read_options.total_order_seek || read_options.auto_prefix_mode || - read_options.prefix_same_as_start) && - prefix_extractor != nullptr; + GetIteratorPrefixMode(read_options, prefix_extractor != nullptr) != + IteratorPrefixMode::kTotalOrder; // Runs the check that the iterator's first Seek() would run, with the same // arguments. auto first_seek_prefix_check = diff --git a/table/plain/plain_table_reader.cc b/table/plain/plain_table_reader.cc index 20a902ed92d..ffd5fbdb26d 100644 --- a/table/plain/plain_table_reader.cc +++ b/table/plain/plain_table_reader.cc @@ -12,6 +12,7 @@ #include "memory/arena.h" #include "monitoring/histogram.h" #include "monitoring/perf_context_imp.h" +#include "options/options_helper.h" #include "rocksdb/cache.h" #include "rocksdb/comparator.h" #include "rocksdb/env.h" @@ -206,8 +207,8 @@ InternalIterator* PlainTableReader::NewIterator( // Auto prefix mode is not implemented in PlainTable. bool use_prefix_seek = !IsTotalOrderMode() && - (options.prefix_same_as_start || - (!options.total_order_seek && !options.auto_prefix_mode)); + GetIteratorPrefixMode(options, /*has_prefix_extractor=*/true) == + IteratorPrefixMode::kPrefix; if (arena == nullptr) { return new PlainTableIterator(this, use_prefix_seek); } else { diff --git a/tools/db_crashtest.py b/tools/db_crashtest.py index df59ef57a02..d565247c44b 100644 --- a/tools/db_crashtest.py +++ b/tools/db_crashtest.py @@ -508,6 +508,7 @@ def apply_random_seed_per_iteration(): "write_dbid_to_manifest": random.randint(0, 1), "track_options_file_number_in_manifest": lambda: random.randint(0, 1), "write_identity_file": random.randint(0, 1), + "prefix_seek_opt_in_only": lambda: random.randint(0, 1), "optimize_manifest_for_recovery": lambda: 1 if random.randint(1, 5) == 1 else 0, "reuse_manifest_on_open": lambda: 1 if random.randint(1, 5) == 1 else 0, "avoid_flush_during_recovery": lambda: random.choice( diff --git a/unreleased_history/behavior_changes/prefix_same_as_start_unbounded_seek_to_first.md b/unreleased_history/behavior_changes/prefix_same_as_start_unbounded_seek_to_first.md new file mode 100644 index 00000000000..40c6ec63314 --- /dev/null +++ b/unreleased_history/behavior_changes/prefix_same_as_start_unbounded_seek_to_first.md @@ -0,0 +1 @@ +With `DBOptions::prefix_seek_opt_in_only`, a `ReadOptions::prefix_same_as_start` iterator now returns `Status::NotSupported` from `SeekToFirst()` and `SeekToLast()`, with or without iterator bounds, and `Status::InvalidArgument` from `Seek()` and `SeekForPrev()` with a target outside the prefix extractor's domain. Use `Seek()` or `SeekForPrev()` to a key in the wanted prefix instead. Without `prefix_seek_opt_in_only`, such scans keep their results, but an iterator using prefix seek no longer creates read-path range tombstones (`min_tombstones_for_range_conversion`) from scans started by `SeekToFirst()` without `iterate_lower_bound`, `SeekToLast()` without `iterate_upper_bound`, or an out-of-domain seek target, because they can skip live keys. diff --git a/unreleased_history/bug_fixes/prefix_same_as_start_with_total_order_seek.md b/unreleased_history/bug_fixes/prefix_same_as_start_with_total_order_seek.md new file mode 100644 index 00000000000..60aaedfcd8b --- /dev/null +++ b/unreleased_history/bug_fixes/prefix_same_as_start_with_total_order_seek.md @@ -0,0 +1 @@ +Fixed a bug where an iterator with `ReadOptions::prefix_same_as_start` combined with `ReadOptions::total_order_seek` or `ReadOptions::auto_prefix_mode`, including every `prefix_same_as_start` iterator under `DBOptions::prefix_seek_opt_in_only`, could skip keys within the seek prefix after changing direction (`Prev()` then `Next()`, or the reverse) when prefix filters are used. `prefix_same_as_start` now takes precedence over `total_order_seek` and `auto_prefix_mode`.