From d5b24fc1147e6373734dcd1d49887f28381e6c77 Mon Sep 17 00:00:00 2001 From: Maciej Szeszko Date: Tue, 7 Jan 2025 08:53:42 -0800 Subject: [PATCH 1/3] Clean up obsolete code in BlockBasedTable::PrefetchIndexAndFilterBlocks --- table/block_based/block_based_table_reader.cc | 69 +++---------------- table/block_based/filter_policy.cc | 4 -- table/block_based/filter_policy_internal.h | 1 - 3 files changed, 11 insertions(+), 63 deletions(-) diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 8a3881d3882..3db08519735 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -1107,71 +1107,24 @@ Status BlockBasedTable::PrefetchIndexAndFilterBlocks( // Find filter handle and filter type if (rep_->filter_policy) { auto name = rep_->filter_policy->CompatibilityName(); - bool builtin_compatible = - strcmp(name, BuiltinFilterPolicy::kCompatibilityName()) == 0; - for (const auto& [filter_type, prefix] : {std::make_pair(Rep::FilterType::kFullFilter, kFullFilterBlockPrefix), std::make_pair(Rep::FilterType::kPartitionedFilter, kPartitionedFilterBlockPrefix), std::make_pair(Rep::FilterType::kNoFilter, kObsoleteFilterBlockPrefix)}) { - if (builtin_compatible) { - // This code is only here to deal with a hiccup in early 7.0.x where - // there was an unintentional name change in the SST files metadata. - // It should be OK to remove this in the future (late 2022) and just - // have the 'else' code. - // NOTE: the test:: names below are likely not needed but included - // out of caution - static const std::unordered_set kBuiltinNameAndAliases = { - BuiltinFilterPolicy::kCompatibilityName(), - test::LegacyBloomFilterPolicy::kClassName(), - test::FastLocalBloomFilterPolicy::kClassName(), - test::Standard128RibbonFilterPolicy::kClassName(), - "rocksdb.internal.DeprecatedBlockBasedBloomFilter", - BloomFilterPolicy::kClassName(), - RibbonFilterPolicy::kClassName(), - }; - - // For efficiency, do a prefix seek and see if the first match is - // good. - meta_iter->Seek(prefix); - if (meta_iter->status().ok() && meta_iter->Valid()) { - Slice key = meta_iter->key(); - if (key.starts_with(prefix)) { - key.remove_prefix(prefix.size()); - if (kBuiltinNameAndAliases.find(key.ToString()) != - kBuiltinNameAndAliases.end()) { - Slice v = meta_iter->value(); - Status s = rep_->filter_handle.DecodeFrom(&v); - if (s.ok()) { - rep_->filter_type = filter_type; - if (filter_type == Rep::FilterType::kNoFilter) { - ROCKS_LOG_WARN(rep_->ioptions.logger, - "Detected obsolete filter type in %s. Read " - "performance might suffer until DB is fully " - "re-compacted.", - rep_->file->file_name().c_str()); - } - break; - } - } - } - } - } else { - std::string filter_block_key = prefix + name; - if (FindMetaBlock(meta_iter, filter_block_key, &rep_->filter_handle) - .ok()) { - rep_->filter_type = filter_type; - if (filter_type == Rep::FilterType::kNoFilter) { - ROCKS_LOG_WARN( - rep_->ioptions.logger, - "Detected obsolete filter type in %s. Read performance might " - "suffer until DB is fully re-compacted.", - rep_->file->file_name().c_str()); - } - break; + std::string filter_block_key = prefix + name; + if (FindMetaBlock(meta_iter, filter_block_key, &rep_->filter_handle) + .ok()) { + rep_->filter_type = filter_type; + if (filter_type == Rep::FilterType::kNoFilter) { + ROCKS_LOG_WARN( + rep_->ioptions.logger, + "Detected obsolete filter type in %s. Read performance might " + "suffer until DB is fully re-compacted.", + rep_->file->file_name().c_str()); } + break; } } } diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index 049148a8ade..3df973aa4ca 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -1410,10 +1410,6 @@ bool BuiltinFilterPolicy::IsInstanceOf(const std::string& name) const { static const char* kBuiltinFilterMetadataName = "rocksdb.BuiltinBloomFilter"; -const char* BuiltinFilterPolicy::kCompatibilityName() { - return kBuiltinFilterMetadataName; -} - const char* BuiltinFilterPolicy::CompatibilityName() const { return kBuiltinFilterMetadataName; } diff --git a/table/block_based/filter_policy_internal.h b/table/block_based/filter_policy_internal.h index 0583274835c..a823bf05973 100644 --- a/table/block_based/filter_policy_internal.h +++ b/table/block_based/filter_policy_internal.h @@ -149,7 +149,6 @@ class BuiltinFilterPolicy : public FilterPolicy { bool IsInstanceOf(const std::string& id) const override; // All variants of BuiltinFilterPolicy can read each others filters. const char* CompatibilityName() const override; - static const char* kCompatibilityName(); public: // new // An internal function for the implementation of From 3464a08a007b69de3e51caf5a5cd52c83374c552 Mon Sep 17 00:00:00 2001 From: Maciej Szeszko Date: Wed, 8 Jan 2025 09:50:09 -0800 Subject: [PATCH 2/3] Add release note for users running on earlier versions --- .../bug_fixes/cleanup_obsolete_code_in_prefetch_index.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md diff --git a/unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md b/unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md new file mode 100644 index 00000000000..f96fb4ff392 --- /dev/null +++ b/unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md @@ -0,0 +1 @@ +Clean up obsolete fix in `BlockBasedTable::PrefetchIndexAndFilterBlocks` isolated to early 7.0.x releases. From c6313943e37270e653435abf8c48774c84994ccc Mon Sep 17 00:00:00 2001 From: Maciej Szeszko Date: Wed, 8 Jan 2025 13:40:44 -0800 Subject: [PATCH 3/3] Revert "Add release note for users running on earlier versions" This reverts commit 39cffed77f4d37307850a198aa3104d37be39089. --- .../bug_fixes/cleanup_obsolete_code_in_prefetch_index.md | 1 - 1 file changed, 1 deletion(-) delete mode 100644 unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md diff --git a/unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md b/unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md deleted file mode 100644 index f96fb4ff392..00000000000 --- a/unreleased_history/bug_fixes/cleanup_obsolete_code_in_prefetch_index.md +++ /dev/null @@ -1 +0,0 @@ -Clean up obsolete fix in `BlockBasedTable::PrefetchIndexAndFilterBlocks` isolated to early 7.0.x releases.