From fff302d989c1a84b7a9434a625ca49d045d569f4 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Thu, 16 Jun 2022 16:41:25 -0700 Subject: [PATCH] More testing w/prefix extractor, small refactor (#10122) Summary: There was an interesting code path not covered by testing that is difficult to replicate in a unit test, which is now covered using a sync point. Specifically, the case of table_prefix_extractor == null and !need_upper_bound_check in `BlockBasedTable::PrefixMayMatch`, which can happen if table reader is open before extractor is registered with global object registry, but is later registered and re-set with SetOptions. (We don't have sufficient testing control over object registry to set that up repeatedly.) Also, this function has been renamed to `PrefixRangeMayMatch` for clarity vs. other functions that are not the same. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10122 Test Plan: unit tests expanded Reviewed By: siying Differential Revision: D36944834 Pulled By: pdillinger fbshipit-source-id: 9e52d9da1929a3e42bbc230fcdc3599949de7bdb --- db/db_bloom_filter_test.cc | 46 ++++++++++++++++++- .../block_based/block_based_table_iterator.h | 6 +-- table/block_based/block_based_table_reader.cc | 2 +- table/block_based/block_based_table_reader.h | 10 ++-- 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index a2a2a7a21..43ed408d9 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -254,6 +254,15 @@ TEST_F(DBBloomFilterTest, GetFilterByPrefixBloomCustomPrefixExtractor) { (*(get_perf_context()->level_to_perf_context))[0].bloom_filter_useful); #endif // ROCKSDB_LITE + // No bloom on extractor changed, after re-open + options.prefix_extractor.reset(NewCappedPrefixTransform(10)); + Reopen(options); + ASSERT_EQ("NOT_FOUND", Get("foobarbar")); + ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_USEFUL), 3); + ASSERT_EQ( + 3, + (*(get_perf_context()->level_to_perf_context))[0].bloom_filter_useful); + get_perf_context()->Reset(); } } @@ -2763,9 +2772,24 @@ TEST_F(DBBloomFilterTest, DynamicBloomFilterUpperBound) { ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_CHECKED), 4); ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_USEFUL), 0); } - ASSERT_OK(dbfull()->SetOptions({{"prefix_extractor", "capped:4"}})); + // Same with re-open + options.prefix_extractor.reset(NewFixedPrefixTransform(3)); + Reopen(options); + { + Slice upper_bound("abd"); + ReadOptions read_options; + read_options.prefix_same_as_start = true; + read_options.iterate_upper_bound = &upper_bound; + std::unique_ptr iter(db_->NewIterator(read_options)); + ASSERT_EQ(CountIter(iter, "abc"), 4); + ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_CHECKED), + 2 + using_full_builder * 2); + ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_USEFUL), 0); + } + // Set back to capped:4 and verify BF is always read + options.prefix_extractor.reset(NewCappedPrefixTransform(4)); + Reopen(options); { - // set back to capped:4 and verify BF is always read Slice upper_bound("abd"); ReadOptions read_options; read_options.prefix_same_as_start = true; @@ -2775,6 +2799,24 @@ TEST_F(DBBloomFilterTest, DynamicBloomFilterUpperBound) { ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_CHECKED), 5); ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_USEFUL), 1); } + // Same if there's a problem initally loading prefix transform + SyncPoint::GetInstance()->SetCallBack( + "BlockBasedTable::Open::ForceNullTablePrefixExtractor", + [&](void* arg) { *static_cast(arg) = true; }); + SyncPoint::GetInstance()->EnableProcessing(); + Reopen(options); + { + Slice upper_bound("abd"); + ReadOptions read_options; + read_options.prefix_same_as_start = true; + read_options.iterate_upper_bound = &upper_bound; + std::unique_ptr iter(db_->NewIterator(read_options)); + ASSERT_EQ(CountIter(iter, "abc"), 0); + ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_CHECKED), + 4 + using_full_builder * 2); + ASSERT_EQ(TestGetTickerCount(options, BLOOM_FILTER_PREFIX_USEFUL), 2); + } + SyncPoint::GetInstance()->DisableProcessing(); } } diff --git a/table/block_based/block_based_table_iterator.h b/table/block_based/block_based_table_iterator.h index 9ba8ecdcc..7937104ba 100644 --- a/table/block_based/block_based_table_iterator.h +++ b/table/block_based/block_based_table_iterator.h @@ -265,9 +265,9 @@ class BlockBasedTableIterator : public InternalIteratorBase { // check. return true; } - if (check_filter_ && - !table_->PrefixMayMatch(ikey, read_options_, prefix_extractor_, - need_upper_bound_check_, &lookup_context_)) { + if (check_filter_ && !table_->PrefixRangeMayMatch( + ikey, read_options_, prefix_extractor_, + need_upper_bound_check_, &lookup_context_)) { // TODO remember the iterator is invalidated because of prefix // match. This can avoid the upper level file iterator to falsely // believe the position is the end of the SST file and move to diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 6c1bb431d..b51711df9 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -1865,7 +1865,7 @@ BlockBasedTable::PartitionedIndexIteratorState::NewSecondaryIterator( // cache. // // REQUIRES: this method shouldn't be called while the DB lock is held. -bool BlockBasedTable::PrefixMayMatch( +bool BlockBasedTable::PrefixRangeMayMatch( const Slice& internal_key, const ReadOptions& read_options, const SliceTransform* options_prefix_extractor, const bool need_upper_bound_check, diff --git a/table/block_based/block_based_table_reader.h b/table/block_based/block_based_table_reader.h index 51981f145..f91e2f717 100644 --- a/table/block_based/block_based_table_reader.h +++ b/table/block_based/block_based_table_reader.h @@ -113,11 +113,11 @@ class BlockBasedTable : public TableReader { size_t max_file_size_for_l0_meta_pin = 0, const std::string& cur_db_session_id = "", uint64_t cur_file_num = 0); - bool PrefixMayMatch(const Slice& internal_key, - const ReadOptions& read_options, - const SliceTransform* options_prefix_extractor, - const bool need_upper_bound_check, - BlockCacheLookupContext* lookup_context) const; + bool PrefixRangeMayMatch(const Slice& internal_key, + const ReadOptions& read_options, + const SliceTransform* options_prefix_extractor, + const bool need_upper_bound_check, + BlockCacheLookupContext* lookup_context) const; // Returns a new iterator over the table contents. // The result of NewIterator() is initially invalid (caller must