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

@@ -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.

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<InternalIterator> range_del_iter(

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
+      // The remaining files we look at will only contain covered keys, so we
-      *status = Status::NotFound();
+      // stop here.
-      return;
+      break;
     }
     if (get_context.sample()) {
       sample_file_read_inc(f->file_metadata);