diff --git a/HISTORY.md b/HISTORY.md index 7dae6eac4..cca59608f 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -12,6 +12,7 @@ * Reverted a behavior change silently introduced in 6.14, in which options parsing/loading functions began returning `NotFound` instead of `InvalidArgument` for option names not available in the present version. * Fixed MultiGet bugs it doesn't return valid data with user defined timestamp. * Fixed a potential bug caused by evaluating `TableBuilder::NeedCompact()` before `TableBuilder::Finish()` in compaction job. For example, the `NeedCompact()` method of `CompactOnDeletionCollector` returned by built-in `CompactOnDeletionCollectorFactory` requires `BlockBasedTable::Finish()` to return the correct result. The bug can cause a compaction-generated file not to be marked for future compaction based on deletion ratio. +* Fixed a seek issue with prefix extractor and timestamp. ### Public API Change * Deprecate `BlockBasedTableOptions::pin_l0_filter_and_index_blocks_in_cache` and `BlockBasedTableOptions::pin_top_level_index_and_filter`. These options still take effect until users migrate to the replacement APIs in `BlockBasedTableOptions::metadata_cache_options`. Migration guidance can be found in the API comments on the deprecated options. diff --git a/db/db_with_timestamp_basic_test.cc b/db/db_with_timestamp_basic_test.cc index 697f3b363..226e42501 100644 --- a/db/db_with_timestamp_basic_test.cc +++ b/db/db_with_timestamp_basic_test.cc @@ -244,6 +244,103 @@ TEST_F(DBBasicTestWithTimestamp, SimpleForwardIterate) { Close(); } +TEST_F(DBBasicTestWithTimestamp, SeekWithPrefixLessThanKey) { + Options options = CurrentOptions(); + options.env = env_; + options.create_if_missing = true; + options.prefix_extractor.reset(NewFixedPrefixTransform(3)); + BlockBasedTableOptions bbto; + bbto.filter_policy.reset(NewBloomFilterPolicy(10, false)); + bbto.cache_index_and_filter_blocks = true; + bbto.whole_key_filtering = true; + options.table_factory.reset(NewBlockBasedTableFactory(bbto)); + const size_t kTimestampSize = Timestamp(0, 0).size(); + TestComparator test_cmp(kTimestampSize); + options.comparator = &test_cmp; + DestroyAndReopen(options); + + WriteOptions write_opts; + std::string ts_str = Timestamp(1, 0); + Slice ts = ts_str; + write_opts.timestamp = &ts; + + ASSERT_OK(db_->Put(write_opts, "foo1", "bar")); + Flush(); + + ASSERT_OK(db_->Put(write_opts, "foo2", "bar")); + Flush(); + + // Move sst file to next level + ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr)); + + ASSERT_OK(db_->Put(write_opts, "foo3", "bar")); + Flush(); + + ReadOptions read_opts; + std::string read_ts = Timestamp(1, 0); + ts = read_ts; + read_opts.timestamp = &ts; + { + std::unique_ptr iter(db_->NewIterator(read_opts)); + iter->Seek("foo"); + ASSERT_TRUE(iter->Valid()); + ASSERT_OK(iter->status()); + iter->Seek("bbb"); + ASSERT_FALSE(iter->Valid()); + ASSERT_OK(iter->status()); + } + + Close(); +} + +TEST_F(DBBasicTestWithTimestamp, SeekWithPrefixLargerThanKey) { + Options options = CurrentOptions(); + options.env = env_; + options.create_if_missing = true; + options.prefix_extractor.reset(NewFixedPrefixTransform(20)); + BlockBasedTableOptions bbto; + bbto.filter_policy.reset(NewBloomFilterPolicy(10, false)); + bbto.cache_index_and_filter_blocks = true; + bbto.whole_key_filtering = true; + options.table_factory.reset(NewBlockBasedTableFactory(bbto)); + const size_t kTimestampSize = Timestamp(0, 0).size(); + TestComparator test_cmp(kTimestampSize); + options.comparator = &test_cmp; + DestroyAndReopen(options); + + WriteOptions write_opts; + std::string ts_str = Timestamp(1, 0); + Slice ts = ts_str; + write_opts.timestamp = &ts; + + ASSERT_OK(db_->Put(write_opts, "foo1", "bar")); + Flush(); + + ASSERT_OK(db_->Put(write_opts, "foo2", "bar")); + Flush(); + + // Move sst file to next level + ASSERT_OK(db_->CompactRange(CompactRangeOptions(), nullptr, nullptr)); + + ASSERT_OK(db_->Put(write_opts, "foo3", "bar")); + Flush(); + + ReadOptions read_opts; + std::string read_ts = Timestamp(2, 0); + ts = read_ts; + read_opts.timestamp = &ts; + { + std::unique_ptr iter(db_->NewIterator(read_opts)); + // Make sure the prefix extractor doesn't include timestamp, otherwise it + // may return invalid result. + iter->Seek("foo"); + ASSERT_TRUE(iter->Valid()); + ASSERT_OK(iter->status()); + } + + Close(); +} + TEST_F(DBBasicTestWithTimestamp, SimpleForwardIterateLowerTsBound) { constexpr int kNumKeysPerFile = 128; constexpr uint64_t kMaxKey = 1024; diff --git a/db/version_set.cc b/db/version_set.cc index f15b94e77..fda3155e5 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1096,13 +1096,17 @@ void LevelIterator::Seek(const Slice& target) { // next key after the prefix, or make the iterator invalid. // A side benefit will be that it invalidates the iterator earlier so that // the upper level merging iterator can merge fewer child iterators. - Slice target_user_key = ExtractUserKey(target); - Slice file_user_key = ExtractUserKey(file_iter_.key()); - if (prefix_extractor_->InDomain(target_user_key) && - (!prefix_extractor_->InDomain(file_user_key) || - user_comparator_.Compare( - prefix_extractor_->Transform(target_user_key), - prefix_extractor_->Transform(file_user_key)) != 0)) { + size_t ts_sz = user_comparator_.timestamp_size(); + Slice target_user_key_without_ts = + ExtractUserKeyAndStripTimestamp(target, ts_sz); + Slice file_user_key_without_ts = + ExtractUserKeyAndStripTimestamp(file_iter_.key(), ts_sz); + if (prefix_extractor_->InDomain(target_user_key_without_ts) && + (!prefix_extractor_->InDomain(file_user_key_without_ts) || + user_comparator_.CompareWithoutTimestamp( + prefix_extractor_->Transform(target_user_key_without_ts), false, + prefix_extractor_->Transform(file_user_key_without_ts), + false) != 0)) { SetFileIterator(nullptr); } }