From e43d2c44244e685f7a1b1a163506b911eb1c49f4 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Mon, 9 Dec 2019 12:19:50 -0800 Subject: [PATCH] Fix & test rocksdb_filterpolicy_create_bloom_full (#6132) Summary: Add overrides needed in FilterPolicy wrapper to fix rocksdb_filterpolicy_create_bloom_full (see issue https://github.com/facebook/rocksdb/issues/6129). Re-enabled assertion in BloomFilterPolicy::CreateFilter that was being violated. Expanded c_test to identify Bloom filter implementations by FP counts. (Without the fix, updated test will trigger assertion and fail otherwise without the assertion.) Fixes https://github.com/facebook/rocksdb/issues/6129 Pull Request resolved: https://github.com/facebook/rocksdb/pull/6132 Test Plan: updated c_test, also run under valgrind. Differential Revision: D18864911 Pulled By: pdillinger fbshipit-source-id: 08e81d7b5368b08e501cd402ef5583f2650c19fa --- db/c.cc | 9 +++++ db/c_test.c | 55 +++++++++++++++++++++++++++--- table/block_based/filter_policy.cc | 3 +- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/db/c.cc b/db/c.cc index 34523f596..e956e6c5f 100644 --- a/db/c.cc +++ b/db/c.cc @@ -3048,6 +3048,15 @@ rocksdb_filterpolicy_t* rocksdb_filterpolicy_create_bloom_format(int bits_per_ke bool KeyMayMatch(const Slice& key, const Slice& filter) const override { return rep_->KeyMayMatch(key, filter); } + // No need to override GetFilterBitsBuilder if this one is overridden + rocksdb::FilterBitsBuilder* GetBuilderWithContext( + const rocksdb::FilterBuildingContext& context) const override { + return rep_->GetBuilderWithContext(context); + } + rocksdb::FilterBitsReader* GetFilterBitsReader( + const Slice& contents) const override { + return rep_->GetFilterBitsReader(contents); + } static void DoNothing(void*) { } }; Wrapper* wrapper = new Wrapper; diff --git a/db/c_test.c b/db/c_test.c index dc62b59ef..e6d7a7f6b 100644 --- a/db/c_test.c +++ b/db/c_test.c @@ -1051,17 +1051,20 @@ int main(int argc, char** argv) { } StartPhase("filter"); - for (run = 0; run < 2; run++) { - // First run uses custom filter, second run uses bloom filter + for (run = 0; run <= 2; run++) { + // First run uses custom filter + // Second run uses old block-based bloom filter + // Third run uses full bloom filter CheckNoError(err); rocksdb_filterpolicy_t* policy; if (run == 0) { policy = rocksdb_filterpolicy_create( NULL, FilterDestroy, FilterCreate, FilterKeyMatch, NULL, FilterName); + } else if (run == 1) { + policy = rocksdb_filterpolicy_create_bloom(8); } else { - policy = rocksdb_filterpolicy_create_bloom(10); + policy = rocksdb_filterpolicy_create_bloom_full(8); } - rocksdb_block_based_options_set_filter_policy(table_options, policy); // Create new database @@ -1074,12 +1077,24 @@ int main(int argc, char** argv) { CheckNoError(err); rocksdb_put(db, woptions, "bar", 3, "barvalue", 8, &err); CheckNoError(err); + + { + // Add enough keys to get just one reasonably populated Bloom filter + const int keys_to_add = 1500; + int i; + char keybuf[100]; + for (i = 0; i < keys_to_add; i++) { + snprintf(keybuf, sizeof(keybuf), "yes%020d", i); + rocksdb_put(db, woptions, keybuf, strlen(keybuf), "val", 3, &err); + CheckNoError(err); + } + } rocksdb_compact_range(db, NULL, 0, NULL, 0); fake_filter_result = 1; CheckGet(db, roptions, "foo", "foovalue"); CheckGet(db, roptions, "bar", "barvalue"); - if (phase == 0) { + if (run == 0) { // Must not find value when custom filter returns false fake_filter_result = 0; CheckGet(db, roptions, "foo", NULL); @@ -1089,6 +1104,36 @@ int main(int argc, char** argv) { CheckGet(db, roptions, "foo", "foovalue"); CheckGet(db, roptions, "bar", "barvalue"); } + + { + // Query some keys not added to identify Bloom filter implementation + // from false positive queries, using perfcontext to detect Bloom + // filter behavior + rocksdb_perfcontext_t* perf = rocksdb_perfcontext_create(); + rocksdb_perfcontext_reset(perf); + + const int keys_to_query = 10000; + int i; + char keybuf[100]; + for (i = 0; i < keys_to_query; i++) { + fake_filter_result = i % 2; + snprintf(keybuf, sizeof(keybuf), "no%020d", i); + CheckGet(db, roptions, keybuf, NULL); + } + + // Other than the first value, these are essentially fingerprints of + // the underlying Bloom filter schemas. + static const int expected_hits[] = {5000, 241, 224}; + CheckCondition( + expected_hits[run] == + (int)rocksdb_perfcontext_metric(perf, rocksdb_bloom_sst_hit_count)); + CheckCondition( + (keys_to_query - expected_hits[run]) == + (int)rocksdb_perfcontext_metric(perf, rocksdb_bloom_sst_miss_count)); + + rocksdb_perfcontext_destroy(perf); + } + // Reset the policy rocksdb_block_based_options_set_filter_policy(table_options, NULL); rocksdb_options_set_block_based_table_factory(options, table_options); diff --git a/table/block_based/filter_policy.cc b/table/block_based/filter_policy.cc index 7d43a8fab..38a6e9f83 100644 --- a/table/block_based/filter_policy.cc +++ b/table/block_based/filter_policy.cc @@ -443,8 +443,7 @@ void BloomFilterPolicy::CreateFilter(const Slice* keys, int n, std::string* dst) const { // We should ideally only be using this deprecated interface for // appropriately constructed BloomFilterPolicy - // FIXME disabled because of bug in C interface; see issue #6129 - //assert(mode_ == kDeprecatedBlock); + assert(mode_ == kDeprecatedBlock); // Compute bloom filter size (in both bits and bytes) uint32_t bits = static_cast(n * whole_bits_per_key_);