Remove deprecated block-based filter (facebook#10184)

Summary:
In facebook#9535, release 7.0, we hid the old block-based filter from being created using
the public API, because of its inefficiency. Although we normally maintain read compatibility
on old DBs forever, filters are not required for reading a DB, only for optimizing read
performance. Thus, it should be acceptable to remove this code and the substantial
maintenance burden it carries as useful features are developed and validated (such
as user timestamp).

This change completely removes the code for reading and writing the old block-based
filters, net removing about 1370 lines of code no longer needed. Options removed from
testing / benchmarking tools. The prior existence is only evident in a couple of places:
* `CacheEntryRole::kDeprecatedFilterBlock` - We can update this public API enum in
a major release to minimize source code incompatibilities.
* A warning is logged when an old table file is opened that used the old block-based
filter. This is provided as a courtesy, and would be a pain to unit test, so manual testing
should suffice. Unfortunately, sst_dump does not tell you whether a file uses
block-based filter, and the structure of the code makes it very difficult to fix.
* To detect that case, `kObsoleteFilterBlockPrefix` (renamed from `kFilterBlockPrefix`)
for metaindex is maintained (for now).

Other notes:
* In some cases where numbers are associated with filter configurations, we have had to
update the assigned numbers so that they all correspond to something that exists.
* Fixed potential stat counting bug by assuming `filter_checked = false` for cases
like `filter == nullptr` rather than assuming `filter_checked = true`
* Removed obsolete `block_offset` and `prefix_extractor` parameters from several
functions.
* Removed some unnecessary checks `if (!table_prefix_extractor() && !prefix_extractor)`
because the caller guarantees the prefix extractor exists and is compatible

Pull Request resolved: facebook#10184

Test Plan:
tests updated, manually test new warning in LOG using base version to
generate a DB

Reviewed By: riversand963

Differential Revision: D37212647

Pulled By: pdillinger

fbshipit-source-id: 06ee020d8de3b81260ffc36ad0c1202cbf463a80

CMakeLists.txt

@@ -738,7 +738,6 @@ set(SOURCES
         table/adaptive/adaptive_table_factory.cc
         table/block_based/binary_search_index_reader.cc
         table/block_based/block.cc
-        table/block_based/block_based_filter_block.cc
         table/block_based/block_based_table_builder.cc
         table/block_based/block_based_table_factory.cc
         table/block_based/block_based_table_iterator.cc
@@ -1321,7 +1320,6 @@ if(WITH_TESTS)
         options/customizable_test.cc
         options/options_settable_test.cc
         options/options_test.cc
-        table/block_based/block_based_filter_block_test.cc
         table/block_based/block_based_table_reader_test.cc
         table/block_based/block_test.cc
         table/block_based/data_block_hash_index_test.cc

HISTORY.md

@@ -37,6 +37,7 @@
 
 ### Behavior changes
 * DB::Open(), DB::OpenAsSecondary() will fail if a Logger cannot be created (#9984)
+* Removed support for reading Bloom filters using obsolete block-based filter format. (Support for writing such filters was dropped in 7.0.) For good read performance on old DBs using these filters, a full compaction is required.
 
 ## 7.3.0 (05/20/2022)
 ### Bug Fixes

Makefile

@@ -1594,9 +1594,6 @@ random_access_file_reader_test: $(OBJ_DIR)/file/random_access_file_reader_test.o
 file_reader_writer_test: $(OBJ_DIR)/util/file_reader_writer_test.o $(TEST_LIBRARY) $(LIBRARY)
 	$(AM_LINK)
 
-block_based_filter_block_test: $(OBJ_DIR)/table/block_based/block_based_filter_block_test.o $(TEST_LIBRARY) $(LIBRARY)
-	$(AM_LINK)
-
 block_based_table_reader_test: table/block_based/block_based_table_reader_test.o $(TEST_LIBRARY) $(LIBRARY)
 	$(AM_LINK)
 

TARGETS

@@ -165,7 +165,6 @@ cpp_library_wrapper(name="rocksdb_lib", srcs=[
         "table/adaptive/adaptive_table_factory.cc",
         "table/block_based/binary_search_index_reader.cc",
         "table/block_based/block.cc",
-        "table/block_based/block_based_filter_block.cc",
         "table/block_based/block_based_table_builder.cc",
         "table/block_based/block_based_table_factory.cc",
         "table/block_based/block_based_table_iterator.cc",
@@ -494,7 +493,6 @@ cpp_library_wrapper(name="rocksdb_whole_archive_lib", srcs=[
         "table/adaptive/adaptive_table_factory.cc",
         "table/block_based/binary_search_index_reader.cc",
         "table/block_based/block.cc",
-        "table/block_based/block_based_filter_block.cc",
         "table/block_based/block_based_table_builder.cc",
         "table/block_based/block_based_table_factory.cc",
         "table/block_based/block_based_table_iterator.cc",
@@ -4800,12 +4798,6 @@ cpp_unittest_wrapper(name="blob_garbage_meter_test",
             extra_compiler_flags=[])
 
 
-cpp_unittest_wrapper(name="block_based_filter_block_test",
-            srcs=["table/block_based/block_based_filter_block_test.cc"],
-            deps=[":rocksdb_test_lib"],
-            extra_compiler_flags=[])
-
-
 cpp_unittest_wrapper(name="block_based_table_reader_test",
             srcs=["table/block_based/block_based_table_reader_test.cc"],
             deps=[":rocksdb_test_lib"],

db/db_block_cache_test.cc

@@ -670,7 +670,7 @@ class DBBlockCacheTest1 : public DBTestBase,
 };
 
 INSTANTIATE_TEST_CASE_P(DBBlockCacheTest1, DBBlockCacheTest1,
-                        ::testing::Values(1, 2, 3));
+                        ::testing::Values(1, 2));
 
 TEST_P(DBBlockCacheTest1, WarmCacheWithBlocksDuringFlush) {
   Options options = CurrentOptions();
@@ -686,13 +686,10 @@ TEST_P(DBBlockCacheTest1, WarmCacheWithBlocksDuringFlush) {
       table_options.partition_filters = true;
       table_options.index_type =
           BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch;
-      table_options.filter_policy.reset(NewBloomFilterPolicy(10, false));
+      table_options.filter_policy.reset(NewBloomFilterPolicy(10));
       break;
-    case 2:  // block-based filter
-      table_options.filter_policy.reset(NewBloomFilterPolicy(10, true));
-      break;
-    case 3:  // full filter
-      table_options.filter_policy.reset(NewBloomFilterPolicy(10, false));
+    case 2:  // full filter
+      table_options.filter_policy.reset(NewBloomFilterPolicy(10));
       break;
     default:
       assert(false);

db_stress_tool/db_stress_common.h

@@ -152,7 +152,6 @@ DECLARE_double(experimental_mempurge_threshold);
 DECLARE_bool(enable_write_thread_adaptive_yield);
 DECLARE_int32(reopen);
 DECLARE_double(bloom_bits);
-DECLARE_bool(use_block_based_filter);
 DECLARE_int32(ribbon_starting_level);
 DECLARE_bool(partition_filters);
 DECLARE_bool(optimize_filters_for_memory);

db_stress_tool/db_stress_gflags.cc

@@ -469,10 +469,6 @@ DEFINE_double(bloom_bits, 10,
               "Bloom filter bits per key. "
               "Negative means use default settings.");
 
-DEFINE_bool(use_block_based_filter, false,
-            "use block based filter"
-            "instead of full filter for block based table");
-
 DEFINE_int32(
     ribbon_starting_level, 999,
     "Use Bloom filter on levels below specified and Ribbon beginning on level "

db_stress_tool/db_stress_test_base.cc

@@ -36,16 +36,7 @@ std::shared_ptr<const FilterPolicy> CreateFilterPolicy() {
     return BlockBasedTableOptions().filter_policy;
   }
   const FilterPolicy* new_policy;
-  if (FLAGS_use_block_based_filter) {
-    if (FLAGS_ribbon_starting_level < 999) {
-      fprintf(
-          stderr,
-          "Cannot combine use_block_based_filter and ribbon_starting_level\n");
-      exit(1);
-    } else {
-      new_policy = NewBloomFilterPolicy(FLAGS_bloom_bits, true);
-    }
-  } else if (FLAGS_ribbon_starting_level >= 999) {
+  if (FLAGS_ribbon_starting_level >= 999) {
     // Use Bloom API
     new_policy = NewBloomFilterPolicy(FLAGS_bloom_bits, false);
   } else {

include/rocksdb/cache.h

@@ -553,7 +553,7 @@ enum class CacheEntryRole {
   kFilterBlock,
   // Block-based table metadata block for partitioned filter
   kFilterMetaBlock,
-  // Block-based table deprecated filter block (old "block-based" filter)
+  // OBSOLETE / DEPRECATED: old/removed block-based filter
   kDeprecatedFilterBlock,
   // Block-based table index block
   kIndexBlock,

options/options_test.cc

@@ -999,21 +999,11 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) {
   EXPECT_EQ(bfp->GetMillibitsPerKey(), 4000);
   EXPECT_EQ(bfp->GetWholeBitsPerKey(), 4);
 
-  // Back door way of enabling deprecated block-based Bloom
-  ASSERT_OK(GetBlockBasedTableOptionsFromString(
-      config_options, table_opt,
-      "filter_policy=rocksdb.internal.DeprecatedBlockBasedBloomFilter:4",
-      &new_opt));
-  auto builtin =
-      dynamic_cast<const BuiltinFilterPolicy*>(new_opt.filter_policy.get());
-  EXPECT_EQ(builtin->GetId(),
-            "rocksdb.internal.DeprecatedBlockBasedBloomFilter:4");
-
   // Test configuring using other internal names
   ASSERT_OK(GetBlockBasedTableOptionsFromString(
       config_options, table_opt,
       "filter_policy=rocksdb.internal.LegacyBloomFilter:3", &new_opt));
-  builtin =
+  auto builtin =
       dynamic_cast<const BuiltinFilterPolicy*>(new_opt.filter_policy.get());
   EXPECT_EQ(builtin->GetId(), "rocksdb.internal.LegacyBloomFilter:3");
 

src.mk

@@ -156,7 +156,6 @@ LIB_SOURCES =                                                   \
   table/adaptive/adaptive_table_factory.cc                      \
   table/block_based/binary_search_index_reader.cc               \
   table/block_based/block.cc                                    \
-  table/block_based/block_based_filter_block.cc                 \
   table/block_based/block_based_table_builder.cc                \
   table/block_based/block_based_table_factory.cc                \
   table/block_based/block_based_table_iterator.cc               \
@@ -525,7 +524,6 @@ TEST_MAIN_SOURCES =                                                     \
   options/customizable_test.cc                                          \
   options/options_settable_test.cc                                      \
   options/options_test.cc                                               \
-  table/block_based/block_based_filter_block_test.cc                    \
   table/block_based/block_based_table_reader_test.cc                    \
   table/block_based/block_test.cc                                       \
   table/block_based/data_block_hash_index_test.cc                       \