Fix false removal of tombstone issue in FIFO and kCompactionStyleNone

Summary:
Similar to the bug fixed by https://github.com/facebook/rocksdb/pull/2726, FIFO with compaction and kCompactionStyleNone during user customized CompactFiles() with output level to be 0 can suffer from the same problem. Fix it by leveraging the bottommost_level_ flag.
Closes https://github.com/facebook/rocksdb/pull/2735

Differential Revision: D5626906

Pulled By: siying

fbshipit-source-id: 2b148d0461c61dbd986d74655e384419ae442158

diff --git a/db/compaction.cc b/db/compaction.cc
index 08bfae97338b770aea8a308745eb2d94463e4af2..f4a82ed33e15fec75677a69a479b9958dec82ea0 100644 (file)
--- a/db/compaction.cc
+++ b/db/compaction.cc
@@ -283,32 +283,32 @@ bool Compaction::KeyNotExistsBeyondOutputLevel(
   assert(input_version_ != nullptr);
   assert(level_ptrs != nullptr);
   assert(level_ptrs->size() == static_cast<size_t>(number_levels_));
-  assert(cfd_->ioptions()->compaction_style != kCompactionStyleFIFO);
-  if (cfd_->ioptions()->compaction_style == kCompactionStyleUniversal) {
-    return bottommost_level_;
-  }
-  if (cfd_->ioptions()->compaction_style == kCompactionStyleLevel &&
-      output_level_ == 0) {
-    return false;
-  }
-  // Maybe use binary search to find right entry instead of linear search?
-  const Comparator* user_cmp = cfd_->user_comparator();
-  for (int lvl = output_level_ + 1; lvl < number_levels_; lvl++) {
-    const std::vector<FileMetaData*>& files = input_vstorage_->LevelFiles(lvl);
-    for (; level_ptrs->at(lvl) < files.size(); level_ptrs->at(lvl)++) {
-      auto* f = files[level_ptrs->at(lvl)];
-      if (user_cmp->Compare(user_key, f->largest.user_key()) <= 0) {
-        // We've advanced far enough
-        if (user_cmp->Compare(user_key, f->smallest.user_key()) >= 0) {
-          // Key falls in this file's range, so definitely
-          // exists beyond output level
-          return false;
+  if (cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+    if (output_level_ == 0) {
+      return false;
+    }
+    // Maybe use binary search to find right entry instead of linear search?
+    const Comparator* user_cmp = cfd_->user_comparator();
+    for (int lvl = output_level_ + 1; lvl < number_levels_; lvl++) {
+      const std::vector<FileMetaData*>& files =
+          input_vstorage_->LevelFiles(lvl);
+      for (; level_ptrs->at(lvl) < files.size(); level_ptrs->at(lvl)++) {
+        auto* f = files[level_ptrs->at(lvl)];
+        if (user_cmp->Compare(user_key, f->largest.user_key()) <= 0) {
+          // We've advanced far enough
+          if (user_cmp->Compare(user_key, f->smallest.user_key()) >= 0) {
+            // Key falls in this file's range, so definitely
+            // exists beyond output level
+            return false;
+          }
+          break;
         }
-        break;
       }
     }
+    return true;
+  } else {
+    return bottommost_level_;
   }
-  return true;
 }
 
 // Mark (or clear) each file that is being compacted

diff --git a/db/db_test.cc b/db/db_test.cc
index 70f54250ba95a77da9a50f6b1aca2f51f55b9926..6e93b39b5a4e2a766f529d805cfb36184a1e9bed 100644 (file)
--- a/db/db_test.cc
+++ b/db/db_test.cc
@@ -2806,6 +2806,46 @@ TEST_F(DBTest, FIFOCompactionTestWithCompaction) {
             options.compaction_options_fifo.max_table_files_size);
 }
 
+TEST_F(DBTest, FIFOCompactionStyleWithCompactionAndDelete) {
+  Options options;
+  options.compaction_style = kCompactionStyleFIFO;
+  options.write_buffer_size = 20 << 10;  // 20K
+  options.arena_block_size = 4096;
+  options.compaction_options_fifo.max_table_files_size = 1500 << 10;  // 1MB
+  options.compaction_options_fifo.allow_compaction = true;
+  options.level0_file_num_compaction_trigger = 3;
+  options.compression = kNoCompression;
+  options.create_if_missing = true;
+  options = CurrentOptions(options);
+  DestroyAndReopen(options);
+
+  Random rnd(301);
+  for (int i = 0; i < 3; i++) {
+    // Each file contains a different key which will be dropped later.
+    ASSERT_OK(Put("a" + ToString(i), RandomString(&rnd, 500)));
+    ASSERT_OK(Put("key" + ToString(i), ""));
+    ASSERT_OK(Put("z" + ToString(i), RandomString(&rnd, 500)));
+    Flush();
+    ASSERT_OK(dbfull()->TEST_WaitForCompact());
+  }
+  ASSERT_EQ(NumTableFilesAtLevel(0), 1);
+  for (int i = 0; i < 3; i++) {
+    ASSERT_EQ("", Get("key" + ToString(i)));
+  }
+  for (int i = 0; i < 3; i++) {
+    // Each file contains a different key which will be dropped later.
+    ASSERT_OK(Put("a" + ToString(i), RandomString(&rnd, 500)));
+    ASSERT_OK(Delete("key" + ToString(i)));
+    ASSERT_OK(Put("z" + ToString(i), RandomString(&rnd, 500)));
+    Flush();
+    ASSERT_OK(dbfull()->TEST_WaitForCompact());
+  }
+  ASSERT_EQ(NumTableFilesAtLevel(0), 2);
+  for (int i = 0; i < 3; i++) {
+    ASSERT_EQ("NOT_FOUND", Get("key" + ToString(i)));
+  }
+}
+
 // Check that FIFO-with-TTL is not supported with max_open_files != -1.
 TEST_F(DBTest, FIFOCompactionWithTTLAndMaxOpenFilesTest) {
   Options options;