Fix SIGSEGV

Summary: As a short-term fix, let's go back to previous way of calculating NeedsCompaction(). SIGSEGV happens because NeedsCompaction() can happen before super_version (and thus MutableCFOptions) is initialized.

Test Plan: make check

Reviewers: ljin, sdong, rven, yhchiang

Reviewed By: yhchiang

Subscribers: dhruba, leveldb

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

db/compaction_picker.cc

@@ -677,9 +677,8 @@ Status CompactionPicker::SanitizeCompactionInputFiles(
 }
 #endif  // ROCKSDB_LITE
 
-bool LevelCompactionPicker::NeedsCompaction(
-    const VersionStorageInfo* vstorage,
-    const MutableCFOptions& mutable_cf_options) const {
+bool LevelCompactionPicker::NeedsCompaction(const VersionStorageInfo* vstorage)
+    const {
   for (int i = 0; i <= vstorage->MaxInputLevel(); i++) {
     if (vstorage->CompactionScore(i) >= 1) {
       return true;
@@ -843,16 +842,9 @@ Compaction* LevelCompactionPicker::PickCompactionBySize(
 }
 
 bool UniversalCompactionPicker::NeedsCompaction(
-    const VersionStorageInfo* vstorage,
-    const MutableCFOptions& mutable_cf_options) const {
+    const VersionStorageInfo* vstorage) const {
   const int kLevel0 = 0;
-
-  if (vstorage->LevelFiles(kLevel0).size() <
-      static_cast<size_t>(
-          mutable_cf_options.level0_file_num_compaction_trigger)) {
-    return false;
-  }
-  return true;
+  return vstorage->CompactionScore(kLevel0) >= 1;
 }
 
 // Universal style of compaction. Pick files that are contiguous in
@@ -1254,25 +1246,10 @@ Compaction* UniversalCompactionPicker::PickCompactionUniversalSizeAmp(
   return c;
 }
 
-bool FIFOCompactionPicker::NeedsCompaction(
-    const VersionStorageInfo* vstorage,
-    const MutableCFOptions& mutable_cf_options) const {
+bool FIFOCompactionPicker::NeedsCompaction(const VersionStorageInfo* vstorage)
+    const {
   const int kLevel0 = 0;
-  const std::vector<FileMetaData*>& level_files = vstorage->LevelFiles(kLevel0);
-
-  if (level_files.size() == 0) {
-    return false;
-  }
-
-  uint64_t total_size = 0;
-  for (const auto& file : level_files) {
-    total_size += file->fd.file_size;
-  }
-  if (total_size <= ioptions_.compaction_options_fifo.max_table_files_size) {
-    return false;
-  }
-
-  return true;
+  return vstorage->CompactionScore(kLevel0) >= 1;
 }
 
 Compaction* FIFOCompactionPicker::PickCompaction(

db/compaction_picker.h

@@ -73,9 +73,7 @@ class CompactionPicker {
     return NumberLevels() - 1;
   }
 
-  virtual bool NeedsCompaction(
-      const VersionStorageInfo* vstorage,
-      const MutableCFOptions& cf_options) const = 0;
+  virtual bool NeedsCompaction(const VersionStorageInfo* vstorage) const = 0;
 
   // Sanitize the input set of compaction input files.
   // When the input parameters do not describe a valid compaction, the
@@ -191,9 +189,8 @@ class UniversalCompactionPicker : public CompactionPicker {
     return 0;
   }
 
-  virtual bool NeedsCompaction(
-      const VersionStorageInfo* vstorage,
-      const MutableCFOptions& cf_options) const override;
+  virtual bool NeedsCompaction(const VersionStorageInfo* vstorage) const
+      override;
 
  private:
   // Pick Universal compaction to limit read amplification
@@ -229,9 +226,8 @@ class LevelCompactionPicker : public CompactionPicker {
     return current_num_levels - 2;
   }
 
-  virtual bool NeedsCompaction(
-      const VersionStorageInfo* vstorage,
-      const MutableCFOptions& cf_options) const override;
+  virtual bool NeedsCompaction(const VersionStorageInfo* vstorage) const
+      override;
 
  private:
   // For the specfied level, pick a compaction.
@@ -270,9 +266,8 @@ class FIFOCompactionPicker : public CompactionPicker {
     return 0;
   }
 
-  virtual bool NeedsCompaction(
-      const VersionStorageInfo* vstorage,
-      const MutableCFOptions& cf_options) const override;
+  virtual bool NeedsCompaction(const VersionStorageInfo* vstorage) const
+      override;
 };
 
 class NullCompactionPicker : public CompactionPicker {
@@ -306,9 +301,8 @@ class NullCompactionPicker : public CompactionPicker {
   }
 
   // Always returns false.
-  virtual bool NeedsCompaction(
-      const VersionStorageInfo* vstorage,
-      const MutableCFOptions& cf_options) const override {
+  virtual bool NeedsCompaction(const VersionStorageInfo* vstorage) const
+      override {
     return false;
   }
 };

db/compaction_picker_test.cc

@@ -198,9 +198,8 @@ TEST(CompactionPickerTest, NeedsCompactionLevel) {
       }
       UpdateVersionStorageInfo();
       ASSERT_EQ(vstorage_->CompactionScoreLevel(0), level);
-      ASSERT_EQ(level_compaction_picker.NeedsCompaction(
-            vstorage_.get(), mutable_cf_options_),
-            vstorage_->CompactionScore(0) >= 1);
+      ASSERT_EQ(level_compaction_picker.NeedsCompaction(vstorage_.get()),
+                vstorage_->CompactionScore(0) >= 1);
       // release the version storage
       DeleteVersionStorage();
     }
@@ -212,20 +211,17 @@ TEST(CompactionPickerTest, NeedsCompactionUniversal) {
   UniversalCompactionPicker universal_compaction_picker(
       ioptions_, &icmp_);
   // must return false when there's no files.
-  ASSERT_EQ(universal_compaction_picker.NeedsCompaction(
-          vstorage_.get(), mutable_cf_options_), false);
+  ASSERT_EQ(universal_compaction_picker.NeedsCompaction(vstorage_.get()),
+            false);
 
   // verify the trigger given different number of L0 files.
   for (int i = 1;
-       i <= mutable_cf_options_.level0_file_num_compaction_trigger * 2;
-       ++i) {
+       i <= mutable_cf_options_.level0_file_num_compaction_trigger * 2; ++i) {
     Add(0, i, std::to_string((i + 100) * 1000).c_str(),
-        std::to_string((i + 100) * 1000 + 999).c_str(),
-        1000000, 0, i * 100, i * 100 + 99);
-    ASSERT_EQ(
-        universal_compaction_picker.NeedsCompaction(
-            vstorage_.get(), mutable_cf_options_),
-        i >= mutable_cf_options_.level0_file_num_compaction_trigger);
+        std::to_string((i + 100) * 1000 + 999).c_str(), 1000000, 0, i * 100,
+        i * 100 + 99);
+    ASSERT_EQ(level_compaction_picker.NeedsCompaction(vstorage_.get()),
+              vstorage_->CompactionScore(0) >= 1);
   }
 }
 
@@ -241,8 +237,7 @@ TEST(CompactionPickerTest, NeedsCompactionFIFO) {
   FIFOCompactionPicker fifo_compaction_picker(ioptions_, &icmp_);
 
   // must return false when there's no files.
-  ASSERT_EQ(fifo_compaction_picker.NeedsCompaction(
-      vstorage_.get(), mutable_cf_options_), false);
+  ASSERT_EQ(fifo_compaction_picker.NeedsCompaction(vstorage_.get()), false);
 
   // verify whether compaction is needed based on the current
   // size of L0 files.
@@ -252,10 +247,8 @@ TEST(CompactionPickerTest, NeedsCompactionFIFO) {
         std::to_string((i + 100) * 1000 + 999).c_str(),
         kFileSize, 0, i * 100, i * 100 + 99);
     current_size += kFileSize;
-    ASSERT_EQ(
-        fifo_compaction_picker.NeedsCompaction(
-            vstorage_.get(), mutable_cf_options_),
-        current_size > fifo_options_.max_table_files_size);
+    ASSERT_EQ(level_compaction_picker.NeedsCompaction(vstorage_.get()),
+              vstorage_->CompactionScore(0) >= 1);
   }
 }
 

db/db_impl.cc

@@ -1681,8 +1681,7 @@ void DBImpl::MaybeScheduleFlushOrCompaction() {
     // no need to refcount since we're under a mutex
     for (auto cfd : *versions_->GetColumnFamilySet()) {
       if (cfd->compaction_picker()->NeedsCompaction(
-              cfd->current()->storage_info(),
-              *cfd->GetCurrentMutableCFOptions())) {
+              cfd->current()->storage_info())) {
         is_compaction_needed = true;
         break;
       }

db/internal_stats.cc

@@ -235,8 +235,7 @@ bool InternalStats::GetIntProperty(DBPropertyType property_type,
     case kCompactionPending:
       // 1 if the system already determines at least one compacdtion is needed.
       // 0 otherwise,
-      *value = (cfd_->compaction_picker()->NeedsCompaction(
-          vstorage, *cfd_->GetCurrentMutableCFOptions()) ? 1 : 0);
+      *value = (cfd_->compaction_picker()->NeedsCompaction(vstorage) ? 1 : 0);
       return true;
     case kBackgroundErrors:
       // Accumulated number of  errors in background flushes or compactions.