Fix a seek issue with prefix extractor and timestamp (#7644)

Summary:
During seek, prefix compare should not include timestamp.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/7644

Test Plan: added unittest

Reviewed By: riversand963

Differential Revision: D24772066

Pulled By: jay-zhuang

fbshipit-source-id: 3982655a8bf8da256a738e8497b73b3d9bdac92e
main
Jay Zhuang 4 years ago committed by Facebook GitHub Bot
parent 16d103d35b
commit 18aee7db7e
  1. 1
      HISTORY.md
  2. 97
      db/db_with_timestamp_basic_test.cc
  3. 18
      db/version_set.cc

@ -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. * 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 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 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 ### 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. * 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.

@ -244,6 +244,103 @@ TEST_F(DBBasicTestWithTimestamp, SimpleForwardIterate) {
Close(); 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<Iterator> 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<Iterator> 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) { TEST_F(DBBasicTestWithTimestamp, SimpleForwardIterateLowerTsBound) {
constexpr int kNumKeysPerFile = 128; constexpr int kNumKeysPerFile = 128;
constexpr uint64_t kMaxKey = 1024; constexpr uint64_t kMaxKey = 1024;

@ -1096,13 +1096,17 @@ void LevelIterator::Seek(const Slice& target) {
// next key after the prefix, or make the iterator invalid. // next key after the prefix, or make the iterator invalid.
// A side benefit will be that it invalidates the iterator earlier so that // A side benefit will be that it invalidates the iterator earlier so that
// the upper level merging iterator can merge fewer child iterators. // the upper level merging iterator can merge fewer child iterators.
Slice target_user_key = ExtractUserKey(target); size_t ts_sz = user_comparator_.timestamp_size();
Slice file_user_key = ExtractUserKey(file_iter_.key()); Slice target_user_key_without_ts =
if (prefix_extractor_->InDomain(target_user_key) && ExtractUserKeyAndStripTimestamp(target, ts_sz);
(!prefix_extractor_->InDomain(file_user_key) || Slice file_user_key_without_ts =
user_comparator_.Compare( ExtractUserKeyAndStripTimestamp(file_iter_.key(), ts_sz);
prefix_extractor_->Transform(target_user_key), if (prefix_extractor_->InDomain(target_user_key_without_ts) &&
prefix_extractor_->Transform(file_user_key)) != 0)) { (!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); SetFileIterator(nullptr);
} }
} }

Loading…
Cancel
Save