From 75df72f2a5e829299ee5881641cfa403bd0a0bee Mon Sep 17 00:00:00 2001 From: Kai Liu Date: Sat, 16 Nov 2013 22:59:22 -0800 Subject: [PATCH] Change the logic in KeyMayExist() Summary: Previously in KeyMayExist(), if DB::Get() returns non-Status::OK(), we assumes key may not exist. However, as if index block is not in block cache, Status::Incomplete() will return. Worse still, if options::filter_delete is enabled, we may falsely ignore the "delete" operation: https://github.com/facebook/rocksdb/blob/master/db/write_batch.cc#L217-L220 This diff fixes this bug and will let crash-test pass. Test Plan: Ran: ./db_stress --test_batches_snapshots=1 --ops_per_thread=1000000 --threads=32 --write_buffer_size=4194304 --destroy_db_initially=1 --reopen=0 --readpercent=5 --prefixpercent=45 --writepercent=35 --delpercent=5 --iterpercent=10 --db=/home/kailiu/local/newer --max_key=100000000 --disable_seek_compaction=0 --mmap_read=0 --block_size=16384 --cache_size=1048576 --open_files=500000 --verify_checksum=1 --sync=0 --disable_wal=0 --disable_data_sync=0 --target_file_size_base=2097152 --target_file_size_multiplier=2 --max_write_buffer_number=3 --max_background_compactions=20 --max_bytes_for_level_base=10485760 --filter_deletes=1 Previously we'll see crash happens very soon. Reviewers: igor, dhruba CC: leveldb Differential Revision: https://reviews.facebook.net/D14115 --- db/db_impl.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/db/db_impl.cc b/db/db_impl.cc index 1241f77430..0037b91a0e 100644 --- a/db/db_impl.cc +++ b/db/db_impl.cc @@ -2698,11 +2698,17 @@ bool DBImpl::KeyMayExist(const ReadOptions& options, std::string* value, bool* value_found) { if (value_found != nullptr) { - *value_found = true; // falsify later if key-may-exist but can't fetch value + // falsify later if key-may-exist but can't fetch value + *value_found = true; } ReadOptions roptions = options; roptions.read_tier = kBlockCacheTier; // read from block cache only - return GetImpl(roptions, key, value, value_found).ok(); + auto s = GetImpl(roptions, key, value, value_found); + + // If options.block_cache != nullptr and the index block of the table didn't + // not present in block_cache, the return value will be Status::Incomplete. + // In this case, key may still exist in the table. + return s.ok() || s.IsIncomplete(); } Iterator* DBImpl::NewIterator(const ReadOptions& options) {