diff --git a/table/block_based_table_factory.cc b/table/block_based_table_factory.cc index 3155f3394..3013ade2a 100644 --- a/table/block_based_table_factory.cc +++ b/table/block_based_table_factory.cc @@ -71,6 +71,11 @@ Status BlockBasedTableFactory::SanitizeOptions( return Status::InvalidArgument("Hash index is specified for block-based " "table, but prefix_extractor is not given"); } + if (table_options_.cache_index_and_filter_blocks && + table_options_.no_block_cache) { + return Status::InvalidArgument("Enable cache_index_and_filter_blocks, " + ", but block cache is disabled"); + } return Status::OK(); } diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index 4b2050e03..c973b755e 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -483,8 +483,8 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions, } // Will use block cache for index/filter blocks access? - if (table_options.block_cache && - table_options.cache_index_and_filter_blocks) { + if (table_options.cache_index_and_filter_blocks) { + assert(table_options.block_cache != nullptr); // Hack: Call NewIndexIterator() to implicitly add index to the block_cache unique_ptr iter(new_table->NewIndexIterator(ReadOptions())); s = iter->status(); @@ -506,19 +506,7 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions, // Set filter block if (rep->filter_policy) { - // First try reading full_filter, then reading block_based_filter - for (auto filter_block_prefix : { kFullFilterBlockPrefix, - kFilterBlockPrefix }) { - std::string key = filter_block_prefix; - key.append(rep->filter_policy->Name()); - - BlockHandle handle; - if (FindMetaBlock(meta_iter.get(), key, &handle).ok()) { - rep->filter.reset(ReadFilter(handle, rep, - filter_block_prefix, nullptr)); - break; - } - } + rep->filter.reset(ReadFilter(rep, meta_iter.get(), nullptr)); } } else { delete index_reader; @@ -726,33 +714,43 @@ Status BlockBasedTable::PutDataBlockToCache( } FilterBlockReader* BlockBasedTable::ReadFilter( - const BlockHandle& filter_handle, BlockBasedTable::Rep* rep, - const std::string& filter_block_prefix, size_t* filter_size) { + Rep* rep, Iterator* meta_index_iter, size_t* filter_size) { // TODO: We might want to unify with ReadBlockFromFile() if we start // requiring checksum verification in Table::Open. - ReadOptions opt; - BlockContents block; - if (!ReadBlockContents(rep->file.get(), rep->footer, opt, filter_handle, - &block, rep->ioptions.env, false).ok()) { - return nullptr; - } + for (auto prefix : {kFullFilterBlockPrefix, kFilterBlockPrefix}) { + std::string filter_block_key = prefix; + filter_block_key.append(rep->filter_policy->Name()); + BlockHandle handle; + if (FindMetaBlock(meta_index_iter, filter_block_key, &handle).ok()) { + BlockContents block; + if (!ReadBlockContents(rep->file.get(), rep->footer, ReadOptions(), + handle, &block, rep->ioptions.env, false).ok()) { + // Error reading the block + return nullptr; + } - if (filter_size) { - *filter_size = block.data.size(); - } + if (filter_size) { + *filter_size = block.data.size(); + } - assert(rep->filter_policy); - if (kFilterBlockPrefix == filter_block_prefix) { - return new BlockBasedFilterBlockReader( - rep->ioptions.prefix_extractor, rep->table_options, std::move(block)); - } else if (kFullFilterBlockPrefix == filter_block_prefix) { - auto filter_bits_reader = rep->filter_policy-> - GetFilterBitsReader(block.data); - - if (filter_bits_reader != nullptr) { - return new FullFilterBlockReader(rep->ioptions.prefix_extractor, - rep->table_options, std::move(block), - filter_bits_reader); + assert(rep->filter_policy); + if (kFilterBlockPrefix == prefix) { + return new BlockBasedFilterBlockReader( + rep->ioptions.prefix_extractor, rep->table_options, + std::move(block)); + } else if (kFullFilterBlockPrefix == prefix) { + auto filter_bits_reader = rep->filter_policy-> + GetFilterBitsReader(block.data); + if (filter_bits_reader != nullptr) { + return new FullFilterBlockReader(rep->ioptions.prefix_extractor, + rep->table_options, + std::move(block), + filter_bits_reader); + } + } else { + assert(false); + return nullptr; + } } } return nullptr; @@ -760,8 +758,11 @@ FilterBlockReader* BlockBasedTable::ReadFilter( BlockBasedTable::CachableEntry BlockBasedTable::GetFilter( bool no_io) const { - // filter pre-populated - if (rep_->filter != nullptr) { + // If cache_index_and_filter_blocks is false, filter should be pre-populated. + // We will return rep_->filter anyway. rep_->filter can be nullptr if filter + // read fails at Open() time. We don't want to reload again since it will + // most probably fail again. + if (!rep_->table_options.cache_index_and_filter_blocks) { return {rep_->filter.get(), nullptr /* cache handle */}; } @@ -775,8 +776,7 @@ BlockBasedTable::CachableEntry BlockBasedTable::GetFilter( char cache_key[kMaxCacheKeyPrefixSize + kMaxVarint64Length]; auto key = GetCacheKey(rep_->cache_key_prefix, rep_->cache_key_prefix_size, rep_->footer.metaindex_handle(), - cache_key - ); + cache_key); Statistics* statistics = rep_->ioptions.statistics; auto cache_handle = @@ -797,22 +797,12 @@ BlockBasedTable::CachableEntry BlockBasedTable::GetFilter( auto s = ReadMetaBlock(rep_, &meta, &iter); if (s.ok()) { - // First try reading full_filter, then reading block_based_filter - for (auto filter_block_prefix : {kFullFilterBlockPrefix, - kFilterBlockPrefix}) { - std::string filter_block_key = filter_block_prefix; - filter_block_key.append(rep_->filter_policy->Name()); - BlockHandle handle; - if (FindMetaBlock(iter.get(), filter_block_key, &handle).ok()) { - filter = ReadFilter(handle, rep_, filter_block_prefix, &filter_size); - - if (filter == nullptr) break; // err happen in ReadFilter - assert(filter_size > 0); - cache_handle = block_cache->Insert( - key, filter, filter_size, &DeleteCachedEntry); - RecordTick(statistics, BLOCK_CACHE_ADD); - break; - } + filter = ReadFilter(rep_, iter.get(), &filter_size); + if (filter != nullptr) { + assert(filter_size > 0); + cache_handle = block_cache->Insert( + key, filter, filter_size, &DeleteCachedEntry); + RecordTick(statistics, BLOCK_CACHE_ADD); } } } diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index b272c4d13..a000c6a9a 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -183,10 +183,10 @@ class BlockBasedTable : public TableReader { std::unique_ptr* iter); // Create the filter from the filter block. - static FilterBlockReader* ReadFilter(const BlockHandle& filter_handle, - Rep* rep, - const std::string& filter_block_prefix, - size_t* filter_size = nullptr); + static FilterBlockReader* ReadFilter( + Rep* rep, + Iterator* meta_index_iter, + size_t* filter_size = nullptr); static void SetupCacheKeyPrefix(Rep* rep); diff --git a/table/table_test.cc b/table/table_test.cc index e4657e8cd..c54fb2ff7 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -1461,8 +1461,6 @@ TEST(BlockBasedTableTest, BlockCacheDisabledTest) { options.create_if_missing = true; options.statistics = CreateDBStatistics(); BlockBasedTableOptions table_options; - // Intentionally commented out: table_options.cache_index_and_filter_blocks = - // true; table_options.block_cache = NewLRUCache(1024); table_options.filter_policy.reset(NewBloomFilterPolicy(10)); options.table_factory.reset(new BlockBasedTableFactory(table_options)); @@ -1521,7 +1519,7 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) { c.Finish(options, ioptions, table_options, GetPlainInternalComparator(options.comparator), &keys, &kvmap); // preloading filter/index blocks is prohibited. - auto reader = dynamic_cast(c.GetTableReader()); + auto* reader = dynamic_cast(c.GetTableReader()); ASSERT_TRUE(!reader->TEST_filter_block_preloaded()); ASSERT_TRUE(!reader->TEST_index_reader_preloaded()); @@ -1567,28 +1565,11 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) { // release the iterator so that the block cache can reset correctly. iter.reset(); - // -- PART 2: Open without block cache - table_options.no_block_cache = true; - table_options.block_cache.reset(); - options.table_factory.reset(new BlockBasedTableFactory(table_options)); - options.statistics = CreateDBStatistics(); // reset the stats - const ImmutableCFOptions ioptions1(options); - c.Reopen(ioptions1); - table_options.no_block_cache = false; - - { - iter.reset(c.NewIterator()); - iter->SeekToFirst(); - ASSERT_EQ("key", iter->key().ToString()); - BlockCachePropertiesSnapshot props(options.statistics.get()); - // Nothing is affected at all - props.AssertEqual(0, 0, 0, 0); - } - - // -- PART 3: Open with very small block cache + // -- PART 2: Open with very small block cache // In this test, no block will ever get hit since the block cache is // too small to fit even one entry. table_options.block_cache = NewLRUCache(1); + options.statistics = CreateDBStatistics(); options.table_factory.reset(new BlockBasedTableFactory(table_options)); const ImmutableCFOptions ioptions2(options); c.Reopen(ioptions2); @@ -1598,7 +1579,6 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) { 0, 0, 0); } - { // Both index and data block get accessed. // It first cache index block then data block. But since the cache size @@ -1618,6 +1598,33 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) { props.AssertEqual(2, 0, 0 + 1, // data block miss 0); } + iter.reset(); + + // -- PART 3: Open table with bloom filter enabled but not in SST file + table_options.block_cache = NewLRUCache(4096); + table_options.cache_index_and_filter_blocks = false; + options.table_factory.reset(NewBlockBasedTableFactory(table_options)); + + TableConstructor c3(BytewiseComparator()); + c3.Add("k01", "hello"); + ImmutableCFOptions ioptions3(options); + // Generate table without filter policy + c3.Finish(options, ioptions3, table_options, + GetPlainInternalComparator(options.comparator), &keys, &kvmap); + // Open table with filter policy + table_options.filter_policy.reset(NewBloomFilterPolicy(1)); + options.table_factory.reset(new BlockBasedTableFactory(table_options)); + options.statistics = CreateDBStatistics(); + ImmutableCFOptions ioptions4(options); + ASSERT_OK(c3.Reopen(ioptions4)); + reader = dynamic_cast(c3.GetTableReader()); + ASSERT_TRUE(!reader->TEST_filter_block_preloaded()); + GetContext get_context(options.comparator, nullptr, nullptr, nullptr, + GetContext::kNotFound, Slice(), nullptr, + nullptr, nullptr); + ASSERT_OK(reader->Get(ReadOptions(), "k01", &get_context)); + BlockCachePropertiesSnapshot props(options.statistics.get()); + props.AssertFilterBlockStat(0, 0); } TEST(BlockBasedTableTest, BlockCacheLeak) {