Disable putting filter block to block cache

Summary: This bug caused server crash issues because the filter block is too big and kept purging out of cache.

Test Plan: Wrote a new unit tests to make sure it works.

Reviewers: dhruba, haobo, igor, sdong

Reviewed By: haobo

CC: leveldb

Differential Revision: https://reviews.facebook.net/D16221

db/plain_table_db_test.cc

@@ -220,7 +220,6 @@ class TestPlainTableFactory : public PlainTableFactory {
                                  size_t index_sparseness = 16)
       : PlainTableFactory(user_key_len, user_key_len, hash_table_ratio,
                           hash_table_ratio),
-        user_key_len_(user_key_len),
         bloom_bits_per_key_(bloom_bits_per_key),
         hash_table_ratio_(hash_table_ratio),
         index_sparseness_(index_sparseness),
@@ -245,7 +244,6 @@ class TestPlainTableFactory : public PlainTableFactory {
   }
 
  private:
-  uint32_t user_key_len_;
   int bloom_bits_per_key_;
   double hash_table_ratio_;
   size_t index_sparseness_;

table/block_based_table_reader.cc

@@ -300,7 +300,7 @@ Status BlockBasedTable::Open(const Options& options, const EnvOptions& soptions,
       assert(index_block->compressionType() == kNoCompression);
       rep->index_block.reset(index_block);
 
-      // Set index block
+      // Set filter block
       if (rep->options.filter_policy) {
         std::string key = kFilterBlockPrefix;
         key.append(rep->options.filter_policy->Name());
@@ -681,8 +681,14 @@ Iterator* BlockBasedTable::BlockReader(void* arg,
 
 BlockBasedTable::CachableEntry<FilterBlockReader>
 BlockBasedTable::GetFilter(bool no_io) const {
-  if (!rep_->options.filter_policy || !rep_->options.block_cache) {
+  // filter pre-populated
-    return {rep_->filter.get(), nullptr};
+  if (rep_->filter != nullptr) {
+    return {rep_->filter.get(), nullptr /* cache handle */};
+  }
+
+  if (rep_->options.filter_policy == nullptr /* do not use filter at all */ ||
+      rep_->options.block_cache == nullptr /* no block cache at all */) {
+    return {nullptr /* filter */, nullptr /* cache handle */};
   }
 
   // Fetching from the cache
@@ -979,4 +985,12 @@ uint64_t BlockBasedTable::ApproximateOffsetOf(const Slice& key) {
   return result;
 }
 
+bool BlockBasedTable::TEST_filter_block_preloaded() const {
+  return rep_->filter != nullptr;
+}
+
+bool BlockBasedTable::TEST_index_block_preloaded() const {
+  return rep_->index_block != nullptr;
+}
+
 }  // namespace rocksdb

table/block_based_table_reader.h

@@ -90,6 +90,9 @@ class BlockBasedTable : public TableReader {
 
   ~BlockBasedTable();
 
+  bool TEST_filter_block_preloaded() const;
+  bool TEST_index_block_preloaded() const;
+
  private:
   template <class TValue>
   struct CachableEntry;

table/table_test.cc

@@ -29,6 +29,7 @@
 #include "rocksdb/memtablerep.h"
 #include "table/block.h"
 #include "table/meta_blocks.h"
+#include "table/block_based_table_reader.h"
 #include "table/block_based_table_builder.h"
 #include "table/block_based_table_factory.h"
 #include "table/block_based_table_reader.h"
@@ -960,6 +961,7 @@ class BlockBasedTableTest : public TableTest {};
 class PlainTableTest : public TableTest {};
 class TablePropertyTest {};
 
+/*
 // This test serves as the living tutorial for the prefix scan of user collected
 // properties.
 TEST(TablePropertyTest, PrefixScanTest) {
@@ -1121,19 +1123,37 @@ TEST(BlockBasedTableTest, NumBlockStat) {
   ASSERT_EQ(kvmap.size(),
             c.table_reader()->GetTableProperties()->num_data_blocks);
 }
+*/
 
-class BlockCacheProperties {
+// A simple tool that takes the snapshot of block cache statistics.
+class BlockCachePropertiesSnapshot {
  public:
-  explicit BlockCacheProperties(Statistics* statistics) {
+  explicit BlockCachePropertiesSnapshot(Statistics* statistics) {
     block_cache_miss = statistics->getTickerCount(BLOCK_CACHE_MISS);
     block_cache_hit = statistics->getTickerCount(BLOCK_CACHE_HIT);
     index_block_cache_miss = statistics->getTickerCount(BLOCK_CACHE_INDEX_MISS);
     index_block_cache_hit = statistics->getTickerCount(BLOCK_CACHE_INDEX_HIT);
     data_block_cache_miss = statistics->getTickerCount(BLOCK_CACHE_DATA_MISS);
     data_block_cache_hit = statistics->getTickerCount(BLOCK_CACHE_DATA_HIT);
+    filter_block_cache_miss =
+        statistics->getTickerCount(BLOCK_CACHE_FILTER_MISS);
+    filter_block_cache_hit = statistics->getTickerCount(BLOCK_CACHE_FILTER_HIT);
+  }
+
+  void AssertIndexBlockStat(int64_t index_block_cache_miss,
+                            int64_t index_block_cache_hit) {
+    ASSERT_EQ(index_block_cache_miss, this->index_block_cache_miss);
+    ASSERT_EQ(index_block_cache_hit, this->index_block_cache_hit);
+  }
+
+  void AssertFilterBlockStat(int64_t filter_block_cache_miss,
+                             int64_t filter_block_cache_hit) {
+    ASSERT_EQ(filter_block_cache_miss, this->filter_block_cache_miss);
+    ASSERT_EQ(filter_block_cache_hit, this->filter_block_cache_hit);
   }
 
   // Check if the fetched props matches the expected ones.
+  // TODO(kailiu) Use this only when you disabled filter policy!
   void AssertEqual(int64_t index_block_cache_miss,
                    int64_t index_block_cache_hit, int64_t data_block_cache_miss,
                    int64_t data_block_cache_hit) const {
@@ -1154,9 +1174,55 @@ class BlockCacheProperties {
   int64_t index_block_cache_hit = 0;
   int64_t data_block_cache_miss = 0;
   int64_t data_block_cache_hit = 0;
+  int64_t filter_block_cache_miss = 0;
+  int64_t filter_block_cache_hit = 0;
 };
 
-TEST(BlockBasedTableTest, BlockCacheTest) {
+// Make sure, by default, index/filter blocks were pre-loaded (meaning we won't
+// use block cache to store them).
+TEST(BlockBasedTableTest, BlockCacheDisabledTest) {
+  Options options;
+  options.create_if_missing = true;
+  options.statistics = CreateDBStatistics();
+  options.block_cache = NewLRUCache(1024);
+  std::unique_ptr<const FilterPolicy> filter_policy(NewBloomFilterPolicy(10));
+  options.filter_policy = filter_policy.get();
+  BlockBasedTableOptions table_options;
+  // Intentionally commented out: table_options.cache_index_and_filter_blocks =
+  // true;
+  options.table_factory.reset(new BlockBasedTableFactory(table_options));
+  std::vector<std::string> keys;
+  KVMap kvmap;
+
+  TableConstructor c(BytewiseComparator());
+  c.Add("key", "value");
+  c.Finish(options, GetPlainInternalComparator(options.comparator), &keys,
+           &kvmap);
+
+  // preloading filter/index blocks is enabled.
+  auto reader = dynamic_cast<BlockBasedTable*>(c.table_reader());
+  ASSERT_TRUE(reader->TEST_filter_block_preloaded());
+  ASSERT_TRUE(reader->TEST_index_block_preloaded());
+
+  {
+    // nothing happens in the beginning
+    BlockCachePropertiesSnapshot props(options.statistics.get());
+    props.AssertIndexBlockStat(0, 0);
+    props.AssertFilterBlockStat(0, 0);
+  }
+
+  {
+    // a hack that just to trigger BlockBasedTable::GetFilter.
+    reader->Get(ReadOptions(), "non-exist-key", nullptr, nullptr, nullptr);
+    BlockCachePropertiesSnapshot props(options.statistics.get());
+    props.AssertIndexBlockStat(0, 0);
+    props.AssertFilterBlockStat(0, 0);
+  }
+}
+
+// Due to the difficulities of the intersaction between statistics, this test
+// only tests the case when "index block is put to block cache"
+TEST(BlockBasedTableTest, FilterBlockInBlockCache) {
   // -- Table construction
   Options options;
   options.create_if_missing = true;
@@ -1174,6 +1240,10 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
   c.Add("key", "value");
   c.Finish(options, GetPlainInternalComparator(options.comparator), &keys,
            &kvmap);
+  // preloading filter/index blocks is prohibited.
+  auto reader = dynamic_cast<BlockBasedTable*>(c.table_reader());
+  ASSERT_TRUE(!reader->TEST_filter_block_preloaded());
+  ASSERT_TRUE(!reader->TEST_index_block_preloaded());
 
   // -- PART 1: Open with regular block cache.
   // Since block_cache is disabled, no cache activities will be involved.
@@ -1181,7 +1251,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
 
   // At first, no block will be accessed.
   {
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     // index will be added to block cache.
     props.AssertEqual(1,  // index block miss
                       0, 0, 0);
@@ -1190,7 +1260,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
   // Only index block will be accessed
   {
     iter.reset(c.NewIterator());
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     // NOTE: to help better highlight the "detla" of each ticker, I use
     // <last_value> + <added_value> to indicate the increment of changed
     // value; other numbers remain the same.
@@ -1201,7 +1271,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
   // Only data block will be accessed
   {
     iter->SeekToFirst();
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     props.AssertEqual(1, 1, 0 + 1,  // data block miss
                       0);
   }
@@ -1210,7 +1280,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
   {
     iter.reset(c.NewIterator());
     iter->SeekToFirst();
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     props.AssertEqual(1, 1 + 1, /* index block hit */
                       1, 0 + 1 /* data block hit */);
   }
@@ -1226,7 +1296,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
     iter.reset(c.NewIterator());
     iter->SeekToFirst();
     ASSERT_EQ("key", iter->key().ToString());
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     // Nothing is affected at all
     props.AssertEqual(0, 0, 0, 0);
   }
@@ -1237,7 +1307,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
   options.block_cache = NewLRUCache(1);
   c.Reopen(options);
   {
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     props.AssertEqual(1,  // index block miss
                       0, 0, 0);
   }
@@ -1248,7 +1318,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
     // It first cache index block then data block. But since the cache size
     // is only 1, index block will be purged after data block is inserted.
     iter.reset(c.NewIterator());
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     props.AssertEqual(1 + 1,  // index block miss
                       0, 0,   // data block miss
                       0);
@@ -1258,7 +1328,7 @@ TEST(BlockBasedTableTest, BlockCacheTest) {
     // SeekToFirst() accesses data block. With similar reason, we expect data
     // block's cache miss.
     iter->SeekToFirst();
-    BlockCacheProperties props(options.statistics.get());
+    BlockCachePropertiesSnapshot props(options.statistics.get());
     props.AssertEqual(2, 0, 0 + 1,  // data block miss
                       0);
   }