From ed5aec5ba3ae71479b4bc60800348c036da2e44c Mon Sep 17 00:00:00 2001 From: Abhishek Madan Date: Tue, 20 Nov 2018 13:27:19 -0800 Subject: [PATCH] Fix range tombstone covering short-circuit logic (#4698) Summary: Since a range tombstone seen at one level will cover all keys in the range at lower levels, there was a short-circuiting check in Get that reported a key was not found at most one file after the range tombstone was discovered. However, this was incorrect for merge operands, since a deletion might only cover some merge operands, which implies that the key should be found. This PR fixes this logic in the Version portion of Get, and removes the logic from the MemTable portion of Get, since the perforamnce benefit provided there is minimal. Pull Request resolved: https://github.com/facebook/rocksdb/pull/4698 Differential Revision: D13142484 Pulled By: abhimadan fbshipit-source-id: cbd74537c806032f2bfa564724d01a80df7c8f10 --- HISTORY.md | 3 +++ db/memtable.cc | 4 ---- db/version_set.cc | 6 +++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 0f3f9e5f1..d4d58db9e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -6,6 +6,9 @@ ### New Features * Introduced `Memoryllocator`, which lets the user specify custom memory allocator for block based table. +### Bug Fixes +* Fixed Get correctness bug in the presence of range tombstones where merge operands covered by a range tombstone always result in NotFound. + ## 5.18.0 (11/12/2018) ### New Features * Introduced `PerfContextByLevel` as part of `PerfContext` which allows storing perf context at each level. Also replaced `__thread` with `thread_local` keyword for perf_context. Added per-level perf context for bloom filter and `Get` query. diff --git a/db/memtable.cc b/db/memtable.cc index 5bf55473f..705b8c069 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -730,10 +730,6 @@ bool MemTable::Get(const LookupKey& key, std::string* value, Status* s, // Avoiding recording stats for speed. return false; } - if (*max_covering_tombstone_seq > 0) { - *s = Status::NotFound(); - return true; - } PERF_TIMER_GUARD(get_from_memtable_time); std::unique_ptr range_del_iter( diff --git a/db/version_set.cc b/db/version_set.cc index 0e9fa0af1..1856a6be6 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -1212,9 +1212,9 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k, while (f != nullptr) { if (*max_covering_tombstone_seq > 0) { - // Use empty error message for speed - *status = Status::NotFound(); - return; + // The remaining files we look at will only contain covered keys, so we + // stop here. + break; } if (get_context.sample()) { sample_file_read_inc(f->file_metadata);