Expand KeyMayExist to return the proper value if it can be found in memory and also check block_cache

Summary: Removed KeyMayExistImpl because KeyMayExist demanded Get like semantics now. Removed no_io from memtable and imm because we need the proper value now and shouldn't just stop when we see Merge in memtable. Added checks to block_cache. Updated documentation and unit-test

Test Plan: make all check;db_stress for 1 hour

Reviewers: dhruba, haobo

Reviewed By: dhruba

CC: leveldb

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

db/db_impl.cc

@@ -2093,13 +2093,11 @@ Status DBImpl::Get(const ReadOptions& options,
   return GetImpl(options, key, value);
 }
 
-// If no_io is true, then returns Status::NotFound if key is not in memtable,
-// immutable-memtable and bloom-filters can guarantee that key is not in db,
-// "value" is garbage string if no_io is true
 Status DBImpl::GetImpl(const ReadOptions& options,
                        const Slice& key,
                        std::string* value,
-                       const bool no_io) {
+                       const bool no_io,
+                       bool* value_found) {
   Status s;
 
   StopWatch sw(env_, options_.statistics, DB_GET);
@@ -2128,12 +2126,12 @@ Status DBImpl::GetImpl(const ReadOptions& options,
   // s is both in/out. When in, s could either be OK or MergeInProgress.
   // value will contain the current merge operand in the latter case.
   LookupKey lkey(key, snapshot);
-  if (mem->Get(lkey, value, &s, options_, no_io)) {
+  if (mem->Get(lkey, value, &s, options_)) {
     // Done
-  } else if (imm.Get(lkey, value, &s, options_, no_io)) {
+  } else if (imm.Get(lkey, value, &s, options_)) {
     // Done
   } else {
-    current->Get(options, lkey, value, &s, &stats, options_, no_io);
+    current->Get(options, lkey, value, &s, &stats, options_, no_io,value_found);
     have_stat_update = true;
   }
   mutex_.Lock();
@@ -2223,19 +2221,14 @@ std::vector<Status> DBImpl::MultiGet(const ReadOptions& options,
   return statList;
 }
 
-bool DBImpl::KeyMayExist(const Slice& key) {
+bool DBImpl::KeyMayExist(const ReadOptions& options,
-  return KeyMayExistImpl(key, versions_->LastSequence());
+                         const Slice& key,
-}
+                         std::string* value,
-
+                         bool* value_found) {
-bool DBImpl::KeyMayExistImpl(const Slice& key,
+  if (value_found != nullptr) {
-                             const SequenceNumber read_from_seq) {
+    *value_found = true; // falsify later if key-may-exist but can't fetch value
-  std::string value;
+  }
-  SnapshotImpl read_from_snapshot;
+  return GetImpl(options, key, value, true, value_found).ok();
-  read_from_snapshot.number_ = read_from_seq;
-  ReadOptions ropts;
-  ropts.snapshot = &read_from_snapshot;
-  const Status s = GetImpl(ropts, key, &value, true);
-  return !s.IsNotFound();
 }
 
 Iterator* DBImpl::NewIterator(const ReadOptions& options) {

db/db_impl.h

@@ -50,9 +50,14 @@ class DBImpl : public DB {
                                        const std::vector<Slice>& keys,
                                        std::vector<std::string>* values);
 
-  // Returns false if key can't exist- based on memtable, immutable-memtable and
+  // Returns false if key doesn't exist in the database and true if it may.
-  // bloom-filters; true otherwise. No IO is performed
+  // If value_found is not passed in as null, then return the value if found in
-  virtual bool KeyMayExist(const Slice& key);
+  // memory. On return, if value was found, then value_found will be set to true
+  // , otherwise false.
+  virtual bool KeyMayExist(const ReadOptions& options,
+                           const Slice& key,
+                           std::string* value,
+                           bool* value_found = nullptr);
   virtual Iterator* NewIterator(const ReadOptions&);
   virtual const Snapshot* GetSnapshot();
   virtual void ReleaseSnapshot(const Snapshot* snapshot);
@@ -104,15 +109,6 @@ class DBImpl : public DB {
   // Trigger's a background call for testing.
   void TEST_PurgeObsoleteteWAL();
 
-  // KeyMayExist's internal function, but can be called internally from rocksdb
-  // to check memtable from sequence_number=read_from_seq. This is useful to
-  // check presence of key in db when key's existence is to be also checked in
-  // an incompletely written WriteBatch in memtable. eg. Database doesn't have
-  // key A and WriteBatch=[PutA,B; DelA]. A KeyMayExist called from DelA also
-  // needs to check itself for any PutA to be sure to not drop the delete.
-  bool KeyMayExistImpl(const Slice& key,
-                       const SequenceNumber read_from_seq);
-
  protected:
   Env* const env_;
   const std::string dbname_;
@@ -415,11 +411,13 @@ class DBImpl : public DB {
     std::vector<SequenceNumber>& snapshots,
     SequenceNumber* prev_snapshot);
 
-  // Function that Get and KeyMayExistImpl call with no_io true or false
+  // Function that Get and KeyMayExist call with no_io true or false
+  // Note: 'value_found' from KeyMayExist propagates here
   Status GetImpl(const ReadOptions& options,
                  const Slice& key,
                  std::string* value,
-                 const bool no_io = false);
+                 const bool no_io = false,
+                 bool* value_found = nullptr);
 };
 
 // Sanitize db options.  The caller should delete result.info_log if

db/db_test.cc

@@ -772,34 +772,41 @@ TEST(DBTest, GetEncountersEmptyLevel) {
   } while (ChangeOptions());
 }
 
-// KeyMayExist-API returns false if memtable(s) and in-memory bloom-filters can
+// KeyMayExist can lead to a few false positives, but not false negatives.
-// guarantee that the key doesn't exist in the db, else true. This can lead to
+// To make test deterministic, use a much larger number of bits per key-20 than
-// a few false positives, but not false negatives. To make test deterministic,
+// bits in the key, so that false positives are eliminated
-// use a much larger number of bits per key-20 than bits in the key, so
-// that false positives are eliminated
 TEST(DBTest, KeyMayExist) {
   do {
+    ReadOptions ropts;
+    std::string value;
     Options options = CurrentOptions();
     options.filter_policy = NewBloomFilterPolicy(20);
     Reopen(&options);
 
-    ASSERT_TRUE(!db_->KeyMayExist("a"));
+    ASSERT_TRUE(!db_->KeyMayExist(ropts, "a", &value));
 
     ASSERT_OK(db_->Put(WriteOptions(), "a", "b"));
-    ASSERT_TRUE(db_->KeyMayExist("a"));
+    bool value_found = false;
+    ASSERT_TRUE(db_->KeyMayExist(ropts, "a", &value, &value_found));
+    ASSERT_TRUE(value_found);
+    ASSERT_EQ("b", value);
 
     dbfull()->Flush(FlushOptions());
-    ASSERT_TRUE(db_->KeyMayExist("a"));
+    value.clear();
+    value_found = false;
+    ASSERT_TRUE(db_->KeyMayExist(ropts, "a", &value, &value_found));
+    ASSERT_TRUE(value_found);
+    ASSERT_EQ("b", value);
 
     ASSERT_OK(db_->Delete(WriteOptions(), "a"));
-    ASSERT_TRUE(!db_->KeyMayExist("a"));
+    ASSERT_TRUE(!db_->KeyMayExist(ropts, "a", &value));
 
     dbfull()->Flush(FlushOptions());
     dbfull()->CompactRange(nullptr, nullptr);
-    ASSERT_TRUE(!db_->KeyMayExist("a"));
+    ASSERT_TRUE(!db_->KeyMayExist(ropts, "a", &value));
 
     ASSERT_OK(db_->Delete(WriteOptions(), "c"));
-    ASSERT_TRUE(!db_->KeyMayExist("c"));
+    ASSERT_TRUE(!db_->KeyMayExist(ropts, "c", &value));
 
     delete options.filter_policy;
   } while (ChangeOptions());
@@ -3045,7 +3052,13 @@ class ModelDB: public DB {
                           Status::NotSupported("Not implemented."));
     return s;
   }
-  virtual bool KeyMayExist(const Slice& key) {
+  virtual bool KeyMayExist(const ReadOptions& options,
+                           const Slice& key,
+                           std::string* value,
+                           bool* value_found = nullptr) {
+    if (value_found != nullptr) {
+      *value_found = false;
+    }
     return true; // Not Supported directly
   }
   virtual Iterator* NewIterator(const ReadOptions& options) {

db/memtable.cc

@@ -130,7 +130,7 @@ void MemTable::Add(SequenceNumber s, ValueType type,
 }
 
 bool MemTable::Get(const LookupKey& key, std::string* value, Status* s,
-              const Options& options, const bool check_presence_only) {
+                  const Options& options) {
   Slice memkey = key.memtable_key();
   std::shared_ptr<MemTableRep::Iterator> iter(table_.get()->GetIterator());
   iter->Seek(memkey.data());
@@ -174,10 +174,6 @@ bool MemTable::Get(const LookupKey& key, std::string* value, Status* s,
           return true;
         }
         case kTypeMerge: {
-          if (check_presence_only) {
-            *s = Status::OK();
-            return true;
-          }
           Slice v = GetLengthPrefixedSlice(key_ptr + key_length);
           if (merge_in_progress) {
             merge_operator->Merge(key.user_key(), &v, operand,

db/memtable.h

@@ -73,13 +73,12 @@ class MemTable {
   // If memtable contains a deletion for key, store a NotFound() error
   // in *status and return true.
   // If memtable contains Merge operation as the most recent entry for a key,
-  //   and if check_presence_only is set, return true with Status::OK,
+  //   and the merge process does not stop (not reaching a value or delete),
-  //   else if the merge process does not stop (not reaching a value or delete),
   //   store the current merged result in value and MergeInProgress in s.
   //   return false
   // Else, return false.
   bool Get(const LookupKey& key, std::string* value, Status* s,
-          const Options& options, const bool check_presence_only = false);
+          const Options& options);
 
   // Returns the edits area that is needed for flushing the memtable
   VersionEdit* GetEdits() { return &edit_; }

db/memtablelist.cc

@@ -194,10 +194,10 @@ size_t MemTableList::ApproximateMemoryUsage() {
 // Search all the memtables starting from the most recent one.
 // Return the most recent value found, if any.
 bool MemTableList::Get(const LookupKey& key, std::string* value, Status* s,
-                      const Options& options, const bool check_presence_only) {
+                      const Options& options) {
   for (list<MemTable*>::iterator it = memlist_.begin();
        it != memlist_.end(); ++it) {
-    if ((*it)->Get(key, value, s, options, check_presence_only)) {
+    if ((*it)->Get(key, value, s, options)) {
       return true;
     }
   }

db/memtablelist.h

@@ -70,7 +70,7 @@ class MemTableList {
   // Search all the memtables starting from the most recent one.
   // Return the most recent value found, if any.
   bool Get(const LookupKey& key, std::string* value, Status* s,
-           const Options& options, const bool check_presence_only = false);
+           const Options& options);
 
   // Returns the list of underlying memtables.
   void GetMemTables(std::vector<MemTable*>* list);

db/version_set.cc

@@ -238,6 +238,7 @@ struct Saver {
   SaverState state;
   const Comparator* ucmp;
   Slice user_key;
+  bool* value_found; // Is value set correctly? Used by KeyMayExist
   std::string* value;
   const MergeOperator* merge_operator;
   Logger* logger;
@@ -245,13 +246,17 @@ struct Saver {
 };
 }
 
-// Called from TableCache::Get when bloom-filters can't guarantee that key does
+// Called from TableCache::Get and InternalGet when file/block in which key may
-// not exist and Get is not permitted to do IO to read the data-block and be
+// exist are not there in TableCache/BlockCache respectively. In this case we
-// certain.
+// can't guarantee that key does not exist and are not permitted to do IO to be
-// Set the key as Found and let the caller know that key-may-exist
+// certain.Set the status=kFound and value_found=false to let the caller know
+// that key may exist but is not there in memory
 static void MarkKeyMayExist(void* arg) {
   Saver* s = reinterpret_cast<Saver*>(arg);
   s->state = kFound;
+  if (s->value_found != nullptr) {
+    *(s->value_found) = false;
+  }
 }
 
 static bool SaveValue(void* arg, const Slice& ikey, const Slice& v, bool didIO){
@@ -339,7 +344,8 @@ void Version::Get(const ReadOptions& options,
                   Status *status,
                   GetStats* stats,
                   const Options& db_options,
-                  const bool no_io) {
+                  const bool no_io,
+                  bool* value_found) {
   Slice ikey = k.internal_key();
   Slice user_key = k.user_key();
   const Comparator* ucmp = vset_->icmp_.user_comparator();
@@ -355,6 +361,7 @@ void Version::Get(const ReadOptions& options,
   saver.state = status->ok()? kNotFound : kMerge;
   saver.ucmp = ucmp;
   saver.user_key = user_key;
+  saver.value_found = value_found;
   saver.value = value;
   saver.merge_operator = merge_operator;
   saver.logger = logger.get();

db/version_set.h

@@ -75,7 +75,7 @@ class Version {
   };
   void Get(const ReadOptions&, const LookupKey& key, std::string* val,
            Status* status, GetStats* stats,  const Options& db_option,
-           const bool no_io = false);
+           const bool no_io = false, bool* value_found = nullptr);
 
   // Adds "stats" into the current state.  Returns true if a new
   // compaction may need to be triggered, false otherwise.

db/write_batch.cc

@@ -16,10 +16,12 @@
 
 #include "leveldb/write_batch.h"
 
+#include "leveldb/options.h"
 #include "leveldb/statistics.h"
 #include "db/dbformat.h"
 #include "db/db_impl.h"
 #include "db/memtable.h"
+#include "db/snapshot.h"
 #include "db/write_batch_internal.h"
 #include "util/coding.h"
 #include <stdexcept>
@@ -167,10 +169,17 @@ class MemTableInserter : public WriteBatch::Handler {
     sequence_++;
   }
   virtual void Delete(const Slice& key) {
-    if (filter_deletes_ && !db_->KeyMayExistImpl(key, sequence_)) {
+    if (filter_deletes_) {
+      SnapshotImpl read_from_snapshot;
+      read_from_snapshot.number_ = sequence_;
+      ReadOptions ropts;
+      ropts.snapshot = &read_from_snapshot;
+      std::string value;
+      if (!db_->KeyMayExist(ropts, key, &value)) {
         RecordTick(options_->statistics, NUMBER_FILTERED_DELETES);
         return;
       }
+    }
     mem_->Add(sequence_, kTypeDeletion, key, Slice());
     sequence_++;
   }

include/leveldb/db.h

@@ -104,7 +104,8 @@ class DB {
   //
   // May return some other Status on an error.
   virtual Status Get(const ReadOptions& options,
-                     const Slice& key, std::string* value) = 0;
+                     const Slice& key,
+                     std::string* value) = 0;
 
   // If keys[i] does not exist in the database, then the i'th returned
   // status will be one for which Status::IsNotFound() is true, and
@@ -121,11 +122,19 @@ class DB {
                                        std::vector<std::string>* values) = 0;
 
   // If the key definitely does not exist in the database, then this method
-  // returns false. Otherwise return true. This check is potentially
+  // returns false, else true. If the caller wants to obtain value when the key
-  // lighter-weight than invoking DB::Get(). One way to make this lighter weight
+  // is found in memory, a bool for 'value_found' must be passed. 'value_found'
-  // is to avoid doing any IOs
+  // will be true on return if value has been set properly.
-  // Default implementation here returns true
+  // This check is potentially lighter-weight than invoking DB::Get(). One way
-  virtual bool KeyMayExist(const Slice& key) {
+  // to make this lighter weight is to avoid doing any IOs.
+  // Default implementation here returns true and sets 'value_found' to false
+  virtual bool KeyMayExist(const ReadOptions& options,
+                           const Slice& key,
+                           std::string* value,
+                           bool* value_found = nullptr) {
+    if (value_found != nullptr) {
+      *value_found = false;
+    }
     return true;
   }
 

include/leveldb/options.h

@@ -473,12 +473,10 @@ struct Options {
   // Default: 0
   uint64_t bytes_per_sync;
 
-  // Use bloom-filter for deletes when this is true.
+  // Use KeyMayExist API to filter deletes when this is true.
-  // db->Delete first calls KeyMayExist which checks memtable,immutable-memtable
+  // If KeyMayExist returns false, i.e. the key definitely does not exist, then
-  // and bloom-filters to determine if the key does not exist in the database.
+  // the delete is a noop. KeyMayExist only incurs in-memory look up.
-  // If the key definitely does not exist, then the delete is a noop.KeyMayExist
+  // This optimization avoids writing the delete to storage when appropriate.
-  // only incurs in-memory look up. This optimization avoids writing the delete
-  // to storage when appropriate.
   // Default: false
   bool filter_deletes;
 

table/table.cc

@@ -235,7 +235,8 @@ Iterator* Table::BlockReader(void* arg,
                              const ReadOptions& options,
                              const Slice& index_value,
                              bool* didIO,
-                             bool for_compaction) {
+                             bool for_compaction,
+                             const bool no_io) {
   Table* table = reinterpret_cast<Table*>(arg);
   Cache* block_cache = table->rep_->options.block_cache.get();
   std::shared_ptr<Statistics> statistics = table->rep_->options.statistics;
@@ -264,6 +265,8 @@ Iterator* Table::BlockReader(void* arg,
         block = reinterpret_cast<Block*>(block_cache->Value(cache_handle));
 
         RecordTick(statistics, BLOCK_CACHE_HIT);
+      } else if (no_io) {
+        return nullptr; // Did not find in block_cache and can't do IO
       } else {
         Histograms histogram = for_compaction ?
           READ_BLOCK_COMPACTION_MICROS : READ_BLOCK_GET_MICROS;
@@ -286,7 +289,9 @@ Iterator* Table::BlockReader(void* arg,
 
         RecordTick(statistics, BLOCK_CACHE_MISS);
       }
-    } else {
+    } else if (no_io) {
+      return nullptr; // Could not read from block_cache and can't do IO
+    }else {
       s = ReadBlock(table->rep_->file.get(), options, handle, &block, didIO);
     }
   }
@@ -340,16 +345,17 @@ Status Table::InternalGet(const ReadOptions& options, const Slice& k,
       // cross one data block, we should be fine.
       RecordTick(rep_->options.statistics, BLOOM_FILTER_USEFUL);
       break;
-    } else if (no_io) {
-        // Update Saver.state to Found because we are only looking for whether
-        // bloom-filter can guarantee the key is not there when "no_io"
-        (*mark_key_may_exist)(arg);
-        done = true;
     } else {
       bool didIO = false;
       Iterator* block_iter = BlockReader(this, options, iiter->value(),
-                                         &didIO);
+                                         &didIO, no_io);
 
+      if (no_io && !block_iter) { // couldn't get block from block_cache
+        // Update Saver.state to Found because we are only looking for whether
+        // we can guarantee the key is not there when "no_io" is set
+        (*mark_key_may_exist)(arg);
+        break;
+      }
       for (block_iter->Seek(k); block_iter->Valid(); block_iter->Next()) {
         if (!(*saver)(arg, block_iter->key(), block_iter->value(), didIO)) {
           done = true;

table/table.h

@@ -77,7 +77,8 @@ class Table {
                                const EnvOptions& soptions, const Slice&,
                                bool for_compaction);
   static Iterator* BlockReader(void*, const ReadOptions&, const Slice&,
-                               bool* didIO, bool for_compaction = false);
+                               bool* didIO, bool for_compaction = false,
+                               const bool no_io = false);
 
   // Calls (*handle_result)(arg, ...) repeatedly, starting with the entry found
   // after a call to Seek(key), until handle_result returns false.

tools/db_stress.cc

@@ -180,7 +180,7 @@ static uint32_t FLAGS_log2_keys_per_lock = 2; // implies 2^2 keys per lock
 // Percentage of times we want to purge redundant keys in memory before flushing
 static uint32_t FLAGS_purge_redundant_percent = 50;
 
-// On true, deletes use bloom-filter and drop the delete if key not present
+// On true, deletes use KeyMayExist to drop the delete if key not present
 static bool FLAGS_filter_deletes = false;
 
 // Level0 compaction start trigger

utilities/ttl/db_ttl.cc

@@ -158,8 +158,11 @@ std::vector<Status> DBWithTTL::MultiGet(const ReadOptions& options,
                                supported with TTL"));
 }
 
-bool DBWithTTL::KeyMayExist(const Slice& key) {
+bool DBWithTTL::KeyMayExist(ReadOptions& options,
-  return db_->KeyMayExist(key);
+                            const Slice& key,
+                            std::string* value,
+                            bool* value_found) {
+  return db_->KeyMayExist(options, key, value, value_found);
 }
 
 Status DBWithTTL::Delete(const WriteOptions& wopts, const Slice& key) {

utilities/ttl/db_ttl.h

@@ -33,7 +33,10 @@ class DBWithTTL : public DB, CompactionFilter {
                                        const std::vector<Slice>& keys,
                                        std::vector<std::string>* values);
 
-  virtual bool KeyMayExist(const Slice& key);
+  virtual bool KeyMayExist(ReadOptions& options,
+                           const Slice& key,
+                           std::string* value,
+                           bool* value_found = nullptr);
 
   virtual Status Delete(const WriteOptions& wopts, const Slice& key);