Skip updating CFOptions if no value to update

db/db_bloom_filter_test.cc

@@ -24,6 +24,7 @@
 #include "rocksdb/perf_context.h"
 #include "rocksdb/statistics.h"
 #include "rocksdb/table.h"
+#include "rocksdb/utilities/object_registry.h"
 #include "table/block_based/block_based_table_reader.h"
 #include "table/block_based/filter_policy_internal.h"
 #include "table/format.h"
@@ -1839,7 +1840,26 @@ class TestingContextCustomFilterPolicy
 }  // anonymous namespace
 
 TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) {
-  auto policy = std::make_shared<TestingContextCustomFilterPolicy>(15, 8, 5);
+  int bpk_fifo = 15;
+  int bpk_l0_other = 8;
+  int bpk_otherwise = 5;
+  auto policy = std::make_shared<TestingContextCustomFilterPolicy>(
+      bpk_fifo, bpk_l0_other, bpk_otherwise);
+
+  // Little hack to make PersistRocksDBOptions work
+  ObjectRegistry::Default()
+      ->AddLibrary("db_bloom_filter_test")
+      ->AddFactory<FilterPolicy>(
+          policy->Name(),
+          [&bpk_fifo, &bpk_l0_other, &bpk_otherwise](
+              const std::string& /*uri*/, std::unique_ptr<FilterPolicy>* guard,
+              std::string* /* errmsg */) {
+            std::unordered_map<std::string, std::string> map;
+            guard->reset(new LevelAndStyleCustomFilterPolicy(
+                bpk_fifo, bpk_l0_other, bpk_otherwise));
+            return guard->get();
+          });
+
   Options options;
   for (bool fifo : {true, false}) {
     options = CurrentOptions();

db/db_impl/db_impl.cc

@@ -1280,11 +1280,25 @@ Status DBImpl::SetOptions(
   InstrumentedMutexLock ol(&options_mutex_);
   MutableCFOptions new_options;
   Status s;
-  Status persist_options_status;
+  Status persist_options_status = Status::OK();
   SuperVersionContext sv_context(/* create_superversion */ true);
   {
     auto db_options = GetDBOptions();
     InstrumentedMutexLock l(&mutex_);
+    MutableCFOptions current_options = *cfd->GetCurrentMutableCFOptions();
+    s = GetMutableOptionsFromStrings(current_options, options_map,
+                                     immutable_db_options_.info_log.get(),
+                                     &new_options);
+    if (s.ok() && MutableCFOptionsAreEqual(current_options, new_options)) {
+      ROCKS_LOG_INFO(
+          immutable_db_options_.info_log,
+          "SetOptions() on column family [%s], no values to update "
+          "(All values are the same as current CFOptions). Skip updating.",
+          cfd->GetName().c_str());
+      persist_options_status.PermitUncheckedError();
+      return s;
+    }
+
     s = cfd->SetOptions(db_options, options_map);
     if (s.ok()) {
       new_options = *cfd->GetLatestMutableCFOptions();
@@ -1352,10 +1366,11 @@ Status DBImpl::SetDBOptions(
       new_options.bytes_per_sync = 1024 * 1024;
     }
 
-    if (MutableDBOptionsAreEqual(mutable_db_options_, new_options)) {
-      ROCKS_LOG_INFO(immutable_db_options_.info_log,
-                     "SetDBOptions(), input option value is not changed, "
-                     "skipping updating.");
+    if (s.ok() && MutableDBOptionsAreEqual(mutable_db_options_, new_options)) {
+      ROCKS_LOG_INFO(
+          immutable_db_options_.info_log,
+          "SetDBOptions(), no values to update (All values are the same as "
+          "current DBOptions). Skip updating.");
       persist_options_status.PermitUncheckedError();
       return s;
     }

db/db_options_test.cc

@@ -119,7 +119,7 @@ TEST_F(DBOptionsTest, ImmutableVerifySstUniqueIdInManifest) {
 
 // RocksDB lite don't support dynamic options.
 
-TEST_F(DBOptionsTest, AvoidUpdatingOptions) {
+TEST_F(DBOptionsTest, SkipUpdatingOptionsWhenNoValuesToUpdate) {
   Options options;
   options.env = env_;
   options.max_background_jobs = 4;

options/cf_options.cc

@@ -219,6 +219,17 @@ static Status TableFactoryParseFn(const ConfigOptions& opts,
   return s;
 }
 
+static bool TableFactoryEqualsFn(const ConfigOptions& opts,
+                                 const std::string& /*name*/, const void* addr1,
+                                 const void* addr2, std::string* mismatch) {
+  auto table_factory1 =
+      static_cast<const std::shared_ptr<TableFactory>*>(addr1);
+  auto table_factory2 =
+      static_cast<const std::shared_ptr<TableFactory>*>(addr2);
+  return table_factory1->get()->AreEquivalent(opts, table_factory2->get(),
+                                              mismatch);
+}
+
 const std::string kOptNameBMCompOpts = "bottommost_compression_opts";
 const std::string kOptNameCompOpts = "compression_opts";
 
@@ -355,23 +366,32 @@ static std::unordered_map<std::string, OptionTypeInfo>
           OptionTypeFlags::kMutable}},
         {"table_factory",
          {offsetof(struct MutableCFOptions, table_factory),
-          OptionType::kCustomizable, OptionVerificationType::kByName,
-          OptionTypeFlags::kShared | OptionTypeFlags::kCompareLoose |
+          OptionType::kCustomizable,
+          OptionVerificationType::kByName,
+          OptionTypeFlags::kShared | OptionTypeFlags::kCompareDefault |
               OptionTypeFlags::kStringNameOnly | OptionTypeFlags::kDontPrepare |
               OptionTypeFlags::kMutable,
-          TableFactoryParseFn}},
+          TableFactoryParseFn,
+          {} /* SerializeFn */,
+          TableFactoryEqualsFn}},
         {"block_based_table_factory",
          {offsetof(struct MutableCFOptions, table_factory),
-          OptionType::kCustomizable, OptionVerificationType::kAlias,
-          OptionTypeFlags::kShared | OptionTypeFlags::kCompareLoose |
+          OptionType::kCustomizable,
+          OptionVerificationType::kAlias,
+          OptionTypeFlags::kShared | OptionTypeFlags::kCompareDefault |
               OptionTypeFlags::kMutable,
-          TableFactoryParseFn}},
+          TableFactoryParseFn,
+          {} /* SerializeFn */,
+          TableFactoryEqualsFn}},
         {"plain_table_factory",
          {offsetof(struct MutableCFOptions, table_factory),
-          OptionType::kCustomizable, OptionVerificationType::kAlias,
-          OptionTypeFlags::kShared | OptionTypeFlags::kCompareLoose |
+          OptionType::kCustomizable,
+          OptionVerificationType::kAlias,
+          OptionTypeFlags::kShared | OptionTypeFlags::kCompareDefault |
               OptionTypeFlags::kMutable,
-          TableFactoryParseFn}},
+          TableFactoryParseFn,
+          {} /* SerializeFn */,
+          TableFactoryEqualsFn}},
         {"filter_deletes",
          {0, OptionType::kBoolean, OptionVerificationType::kDeprecated,
           OptionTypeFlags::kMutable}},
@@ -1274,4 +1294,14 @@ std::vector<std::string> TEST_GetImmutableInMutableCFOptions() {
   return result;
 }
 #endif  // !NDEBUG
+
+bool MutableCFOptionsAreEqual(const MutableCFOptions& this_options,
+                              const MutableCFOptions& that_options) {
+  ConfigOptions config_options;
+  std::string mismatch;
+  return OptionTypeInfo::StructsAreEqual(
+      config_options, "MutableCFOptions", &cf_mutable_options_type_info,
+      "MutableCFOptions", &this_options, &that_options, &mismatch);
+}
+
 }  // namespace ROCKSDB_NAMESPACE

options/cf_options.h

@@ -362,4 +362,7 @@ Status GetMutableOptionsFromStrings(
 std::vector<std::string> TEST_GetImmutableInMutableCFOptions();
 #endif
 
+bool MutableCFOptionsAreEqual(const MutableCFOptions& this_options,
+                              const MutableCFOptions& that_options);
+
 }  // namespace ROCKSDB_NAMESPACE

options/options_test.cc

@@ -14,6 +14,7 @@
 
 #include "cache/lru_cache.h"
 #include "cache/sharded_cache.h"
+#include "db/db_test_util.h"
 #include "options/options_helper.h"
 #include "options/options_parser.h"
 #include "port/port.h"
@@ -2347,6 +2348,58 @@ static int RegisterCustomEnv(ObjectLibrary& library, const std::string& arg) {
   return 1;
 }
 
+class CFOptionsTest : public DBTestBase {
+ public:
+  CFOptionsTest() : DBTestBase("cf_options_test", /*env_do_fsync=*/true) {}
+};
+
+TEST_F(CFOptionsTest, SkipUpdatingOptionsWhenNoValuesToUpdate) {
+  Options options;
+
+  // CFOptions
+  options.uncache_aggressiveness = 0;
+  options.disable_auto_compactions = false;
+
+  Reopen(options);
+
+  SyncPoint::GetInstance()->DisableProcessing();
+  SyncPoint::GetInstance()->ClearAllCallBacks();
+  bool is_changed_stats = false;
+  SyncPoint::GetInstance()->SetCallBack(
+      "DBImpl::WriteOptionsFile:PersistOptions", [&](void* /*arg*/) {
+        ASSERT_FALSE(is_changed_stats);  // should only save options file once
+        is_changed_stats = true;
+      });
+  SyncPoint::GetInstance()->EnableProcessing();
+
+  // helper function to check the status and reset after each check
+  auto is_changed = [&] {
+    bool ret = is_changed_stats;
+    is_changed_stats = false;
+    return ret;
+  };
+
+  // Try updating CFOptions to the same value
+  ASSERT_OK(dbfull()->SetOptions({{"uncache_aggressiveness", "0"}}));
+  ASSERT_FALSE(is_changed());
+
+  ASSERT_OK(dbfull()->SetOptions({{"disable_auto_compactions", "false"}}));
+  ASSERT_FALSE(is_changed());
+
+  // now change
+  ASSERT_OK(dbfull()->SetOptions({{"uncache_aggressiveness", "1"}}));
+  ASSERT_TRUE(is_changed());
+
+  // update again
+  ASSERT_OK(dbfull()->SetOptions({{"uncache_aggressiveness", "0"}}));
+  ASSERT_TRUE(is_changed());
+
+  // multiple values with change
+  ASSERT_OK(dbfull()->SetOptions(
+      {{"uncache_aggressiveness", "0"}, {"disable_auto_compactions", "true"}}));
+  ASSERT_TRUE(is_changed());
+}
+
 // This test suite tests the old APIs into the Configure options methods.
 // Once those APIs are officially deprecated, this test suite can be deleted.
 class OptionsOldApiTest : public testing::Test {};

table/block_based/block_based_table_factory.cc

@@ -312,7 +312,7 @@ static struct BlockBasedTableTypeInfo {
         {"filter_policy",
          OptionTypeInfo::AsCustomSharedPtr<const FilterPolicy>(
              offsetof(struct BlockBasedTableOptions, filter_policy),
-             OptionVerificationType::kByNameAllowFromNull)},
+             OptionVerificationType::kByName, OptionTypeFlags::kAllowNull)},
         {"whole_key_filtering",
          {offsetof(struct BlockBasedTableOptions, whole_key_filtering),
           OptionType::kBoolean, OptionVerificationType::kNormal}},

unreleased_history/behavior_changes/skip_set_options_if_value_to_update.md

@@ -0,0 +1 @@
+SetOptions() API won't update the CFOptions (and no new OPTIONS file will be created) if the values to be updated are the same as the current option values.