Clean up some CFOptions code hygiene, fix SetOptions() bug (facebook#…

…13294)

Summary:
To start, I wanted to remove the unnecessary new_options parameter of `InstallSuperVersionAndScheduleWork()`. Passing it something other than the latest mutable options would be inconsistent/outdated. There was even a comment "Use latest MutableCFOptions" on a place that was using the saved options in effect for the compaction.

On investigation, this fixes an undiagnosed but longstanding serious bug in SetOptions() where the new settings can be reverted if a flush or compaction started before the SetOptions() finishes after. Fix confirmed with new unit test in db_test.cc.

I also got tired of seeing the cumbersome usage of pointer rather than const reference for related options accesses, so there's kind of a large (but trivial) refactoring tied in here as well. (Sorry for combining them; wasn't planning a major bug fix)

Intended follow-up: Clarify/simplify the crazy calling conventions of LogAndApply, and remove some unnecessary copying of MutableCFOptions (see new FIXMEs)

Pull Request resolved: facebook#13294

Test Plan: test for bug fix, confirmed fails on main and at least as far back as version 8.10. Plus existing tests and CI

Reviewed By: mszeszko-meta

Differential Revision: D68141563

Pulled By: pdillinger

fbshipit-source-id: f6c3290145afa06cc2fe8b485a5de17560a5deea

db/arena_wrapped_db_iter.cc

@@ -97,7 +97,7 @@ Status ArenaWrappedDBIter::Refresh(const Snapshot* snapshot) {
     if (read_callback_) {
       read_callback_->Refresh(read_seq);
     }
-    Init(env, read_options_, *(cfd->ioptions()), sv->mutable_cf_options,
+    Init(env, read_options_, cfd->ioptions(), sv->mutable_cf_options,
          sv->current, read_seq,
          sv->mutable_cf_options.max_sequential_skip_in_iterations,
          sv->version_number, read_callback_, cfh_, expose_blob_index_,

db/column_family.cc

@@ -53,7 +53,7 @@ ColumnFamilyHandleImpl::ColumnFamilyHandleImpl(
 
 ColumnFamilyHandleImpl::~ColumnFamilyHandleImpl() {
   if (cfd_ != nullptr) {
-    for (auto& listener : cfd_->ioptions()->listeners) {
+    for (auto& listener : cfd_->ioptions().listeners) {
       listener->OnColumnFamilyHandleDeletionStarted(this);
     }
     // Job id == 0 means that this is not our background process, but rather
@@ -593,7 +593,7 @@ ColumnFamilyData::ColumnFamilyData(
                                       block_cache_tracer, io_tracer,
                                       db_session_id));
     blob_file_cache_.reset(
-        new BlobFileCache(_table_cache, ioptions(), soptions(), id_,
+        new BlobFileCache(_table_cache, &ioptions(), soptions(), id_,
                           internal_stats_->GetBlobFileReadHist(), io_tracer));
     blob_source_.reset(new BlobSource(ioptions_, mutable_cf_options_, db_id,
                                       db_session_id, blob_file_cache_.get()));
@@ -968,7 +968,7 @@ WriteStallCondition ColumnFamilyData::RecalculateWriteStallConditions(
     auto write_stall_condition_and_cause = GetWriteStallConditionAndCause(
         imm()->NumNotFlushed(), vstorage->l0_delay_trigger_count(),
         vstorage->estimated_compaction_needed_bytes(), mutable_cf_options,
-        *ioptions());
+        ioptions());
     write_stall_condition = write_stall_condition_and_cause.first;
     auto write_stall_cause = write_stall_condition_and_cause.second;
 
@@ -1384,7 +1384,7 @@ void ColumnFamilyData::InstallSuperVersion(
         new_superversion->write_stall_condition) {
       sv_context->PushWriteStallNotification(
           old_superversion->write_stall_condition,
-          new_superversion->write_stall_condition, GetName(), ioptions());
+          new_superversion->write_stall_condition, GetName(), &ioptions());
     }
     if (old_superversion->Unref()) {
       old_superversion->Cleanup();
@@ -1844,10 +1844,7 @@ const ImmutableOptions& GetImmutableOptions(ColumnFamilyHandle* column_family) {
   const ColumnFamilyData* const cfd = handle->cfd();
   assert(cfd);
 
-  const ImmutableOptions* ioptions = cfd->ioptions();
-  assert(ioptions);
-
-  return *ioptions;
+  return cfd->ioptions();
 }
 
 }  // namespace ROCKSDB_NAMESPACE

db/column_family.h

@@ -221,7 +221,8 @@ struct SuperVersion {
   // enable UDT feature, this is an empty string.
   std::string full_history_ts_low;
 
-  // A shared copy of the DB's seqno to time mapping.
+  // An immutable snapshot of the DB's seqno to time mapping, usually shared
+  // between SuperVersions.
   std::shared_ptr<const SeqnoToTimeMapping> seqno_to_time_mapping{nullptr};
 
   // should be called outside the mutex
@@ -342,17 +343,17 @@ class ColumnFamilyData {
 
   // thread-safe
   const FileOptions* soptions() const;
-  const ImmutableOptions* ioptions() const { return &ioptions_; }
+  const ImmutableOptions& ioptions() const { return ioptions_; }
   // REQUIRES: DB mutex held
   // This returns the MutableCFOptions used by current SuperVersion
   // You should use this API to reference MutableCFOptions most of the time.
-  const MutableCFOptions* GetCurrentMutableCFOptions() const {
-    return &(super_version_->mutable_cf_options);
+  const MutableCFOptions& GetCurrentMutableCFOptions() const {
+    return super_version_->mutable_cf_options;
   }
   // REQUIRES: DB mutex held
   // This returns the latest MutableCFOptions, which may be not in effect yet.
-  const MutableCFOptions* GetLatestMutableCFOptions() const {
-    return &mutable_cf_options_;
+  const MutableCFOptions& GetLatestMutableCFOptions() const {
+    return mutable_cf_options_;
   }
 
   // REQUIRES: DB mutex held

db/compaction/compaction.cc

@@ -332,11 +332,11 @@ Compaction::Compaction(
               : (_blob_garbage_collection_policy ==
                          BlobGarbageCollectionPolicy::kDisable
                      ? false
-                     : mutable_cf_options()->enable_blob_garbage_collection)),
+                     : mutable_cf_options().enable_blob_garbage_collection)),
       blob_garbage_collection_age_cutoff_(
           _blob_garbage_collection_age_cutoff < 0 ||
                   _blob_garbage_collection_age_cutoff > 1
-              ? mutable_cf_options()->blob_garbage_collection_age_cutoff
+              ? mutable_cf_options().blob_garbage_collection_age_cutoff
               : _blob_garbage_collection_age_cutoff),
       penultimate_level_(
           // For simplicity, we don't support the concept of "penultimate level"
@@ -592,7 +592,7 @@ bool Compaction::IsTrivialMove() const {
   // input files are non overlapping
   if ((mutable_cf_options_.compaction_options_universal.allow_trivial_move) &&
       (output_level_ != 0) &&
-      (cfd_->ioptions()->compaction_style == kCompactionStyleUniversal)) {
+      (cfd_->ioptions().compaction_style == kCompactionStyleUniversal)) {
     return is_trivial_move_;
   }
 
@@ -650,7 +650,7 @@ bool Compaction::KeyNotExistsBeyondOutputLevel(
   if (bottommost_level_) {
     return true;
   } else if (output_level_ != 0 &&
-             cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+             cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     // 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++) {
@@ -691,7 +691,7 @@ bool Compaction::KeyRangeNotExistsBeyondOutputLevel(
   if (bottommost_level_) {
     return true /* does not overlap */;
   } else if (output_level_ != 0 &&
-             cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+             cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     const Comparator* user_cmp = cfd_->user_comparator();
     for (int lvl = output_level_ + 1; lvl < number_levels_; lvl++) {
       const std::vector<FileMetaData*>& files =
@@ -867,12 +867,12 @@ uint64_t Compaction::OutputFilePreallocationSize() const {
 }
 
 std::unique_ptr<CompactionFilter> Compaction::CreateCompactionFilter() const {
-  if (!cfd_->ioptions()->compaction_filter_factory) {
+  if (!cfd_->ioptions().compaction_filter_factory) {
     return nullptr;
   }
 
   if (!cfd_->ioptions()
-           ->compaction_filter_factory->ShouldFilterTableFileCreation(
+           .compaction_filter_factory->ShouldFilterTableFileCreation(
                TableFileCreationReason::kCompaction)) {
     return nullptr;
   }
@@ -891,7 +891,7 @@ std::unique_ptr<CompactionFilter> Compaction::CreateCompactionFilter() const {
         "for compaction.");
   }
 
-  return cfd_->ioptions()->compaction_filter_factory->CreateCompactionFilter(
+  return cfd_->ioptions().compaction_filter_factory->CreateCompactionFilter(
       context);
 }
 
@@ -925,18 +925,18 @@ bool Compaction::ShouldFormSubcompactions() const {
 
   // Round-Robin pri under leveled compaction allows subcompactions by default
   // and the number of subcompactions can be larger than max_subcompactions_
-  if (cfd_->ioptions()->compaction_pri == kRoundRobin &&
-      cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+  if (cfd_->ioptions().compaction_pri == kRoundRobin &&
+      cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     return output_level_ > 0;
   }
 
   if (max_subcompactions_ <= 1) {
     return false;
   }
 
-  if (cfd_->ioptions()->compaction_style == kCompactionStyleLevel) {
+  if (cfd_->ioptions().compaction_style == kCompactionStyleLevel) {
     return (start_level_ == 0 || is_manual_compaction_) && output_level_ > 0;
-  } else if (cfd_->ioptions()->compaction_style == kCompactionStyleUniversal) {
+  } else if (cfd_->ioptions().compaction_style == kCompactionStyleUniversal) {
     return number_levels_ > 1 && output_level_ > 0;
   } else {
     return false;

db/compaction/compaction.h

@@ -288,14 +288,14 @@ class Compaction {
 
   // Return the ImmutableOptions that should be used throughout the compaction
   // procedure
-  const ImmutableOptions* immutable_options() const {
-    return &immutable_options_;
+  const ImmutableOptions& immutable_options() const {
+    return immutable_options_;
   }
 
   // Return the MutableCFOptions that should be used throughout the compaction
   // procedure
-  const MutableCFOptions* mutable_cf_options() const {
-    return &mutable_cf_options_;
+  const MutableCFOptions& mutable_cf_options() const {
+    return mutable_cf_options_;
   }
 
   // Returns the size in bytes that the output file should be preallocated to.

db/compaction/compaction_iterator.h

@@ -124,8 +124,6 @@ class CompactionIterator {
     explicit RealCompaction(const Compaction* compaction)
         : compaction_(compaction) {
       assert(compaction_);
-      assert(compaction_->immutable_options());
-      assert(compaction_->mutable_cf_options());
     }
 
     int level() const override { return compaction_->level(); }
@@ -147,11 +145,11 @@ class CompactionIterator {
     }
 
     bool allow_ingest_behind() const override {
-      return compaction_->immutable_options()->allow_ingest_behind;
+      return compaction_->immutable_options().allow_ingest_behind;
     }
 
     bool allow_mmap_reads() const override {
-      return compaction_->immutable_options()->allow_mmap_reads;
+      return compaction_->immutable_options().allow_mmap_reads;
     }
 
     bool enable_blob_garbage_collection() const override {
@@ -163,7 +161,7 @@ class CompactionIterator {
     }
 
     uint64_t blob_compaction_readahead_size() const override {
-      return compaction_->mutable_cf_options()->blob_compaction_readahead_size;
+      return compaction_->mutable_cf_options().blob_compaction_readahead_size;
     }
 
     const Version* input_version() const override {