From 79790cf2a80fb5e5b6799ebd69d3fb2ebe71d612 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Thu, 26 Sep 2024 14:36:29 -0700 Subject: [PATCH] Bug fix and test BuildDBOptions (#13038) Summary: The following DBOptions were not being propagated through BuildDBOptions, which could at least lead to settings being lost through `GetOptionsFromString()`, possibly elsewhere as well: * background_close_inactive_wals * write_dbid_to_manifest * write_identity_file * prefix_seek_opt_in_only Pull Request resolved: https://github.com/facebook/rocksdb/pull/13038 Test Plan: This problem was not being caught by OptionsSettableTest.DBOptionsAllFieldsSettable when the option was omitted from both options_helper.cc and options_settable_test.cc. I have added to the test to catch future instances (and the updated test was how I found three of the four missing options). The same kind of bug seems to be caught by ColumnFamilyOptionsAllFieldsSettable, and AFAIK analogous code does not exist for BlockBasedTableOptions. Reviewed By: ltamasi Differential Revision: D63483779 Pulled By: pdillinger fbshipit-source-id: a5d5f6e434174bacb8e5d251b767e81e62b7225a --- options/options_helper.cc | 19 +++++++++++++++---- options/options_helper.h | 4 ++++ options/options_settable_test.cc | 12 +++++++++++- .../bug_fixes/build_db_options.md | 1 + 4 files changed, 31 insertions(+), 5 deletions(-) create mode 100644 unreleased_history/bug_fixes/build_db_options.md diff --git a/options/options_helper.cc b/options/options_helper.cc index 011f47b984..232b3f3bd5 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -55,7 +55,13 @@ Status ValidateOptions(const DBOptions& db_opts, DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, const MutableDBOptions& mutable_db_options) { DBOptions options; + BuildDBOptions(immutable_db_options, mutable_db_options, options); + return options; +} +void BuildDBOptions(const ImmutableDBOptions& immutable_db_options, + const MutableDBOptions& mutable_db_options, + DBOptions& options) { options.create_if_missing = immutable_db_options.create_if_missing; options.create_missing_column_families = immutable_db_options.create_missing_column_families; @@ -88,9 +94,6 @@ DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, options.max_background_jobs = mutable_db_options.max_background_jobs; options.max_background_compactions = mutable_db_options.max_background_compactions; - options.bytes_per_sync = mutable_db_options.bytes_per_sync; - options.wal_bytes_per_sync = mutable_db_options.wal_bytes_per_sync; - options.strict_bytes_per_sync = mutable_db_options.strict_bytes_per_sync; options.max_subcompactions = mutable_db_options.max_subcompactions; options.max_background_flushes = mutable_db_options.max_background_flushes; options.max_log_file_size = immutable_db_options.max_log_file_size; @@ -127,6 +130,9 @@ DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, options.writable_file_max_buffer_size = mutable_db_options.writable_file_max_buffer_size; options.use_adaptive_mutex = immutable_db_options.use_adaptive_mutex; + options.bytes_per_sync = mutable_db_options.bytes_per_sync; + options.wal_bytes_per_sync = mutable_db_options.wal_bytes_per_sync; + options.strict_bytes_per_sync = mutable_db_options.strict_bytes_per_sync; options.listeners = immutable_db_options.listeners; options.enable_thread_tracking = immutable_db_options.enable_thread_tracking; options.delayed_write_rate = mutable_db_options.delayed_write_rate; @@ -161,9 +167,15 @@ DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, options.two_write_queues = immutable_db_options.two_write_queues; options.manual_wal_flush = immutable_db_options.manual_wal_flush; options.wal_compression = immutable_db_options.wal_compression; + options.background_close_inactive_wals = + immutable_db_options.background_close_inactive_wals; options.atomic_flush = immutable_db_options.atomic_flush; options.avoid_unnecessary_blocking_io = immutable_db_options.avoid_unnecessary_blocking_io; + options.write_dbid_to_manifest = immutable_db_options.write_dbid_to_manifest; + options.write_identity_file = immutable_db_options.write_identity_file; + options.prefix_seek_opt_in_only = + immutable_db_options.prefix_seek_opt_in_only; options.log_readahead_size = immutable_db_options.log_readahead_size; options.file_checksum_gen_factory = immutable_db_options.file_checksum_gen_factory; @@ -189,7 +201,6 @@ DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, options.metadata_write_temperature = immutable_db_options.metadata_write_temperature; options.wal_write_temperature = immutable_db_options.wal_write_temperature; - return options; } ColumnFamilyOptions BuildColumnFamilyOptions( diff --git a/options/options_helper.h b/options/options_helper.h index 679a1a7eed..7519b627d9 100644 --- a/options/options_helper.h +++ b/options/options_helper.h @@ -44,6 +44,10 @@ Status ValidateOptions(const DBOptions& db_opts, DBOptions BuildDBOptions(const ImmutableDBOptions& immutable_db_options, const MutableDBOptions& mutable_db_options); +// Overwrites `options` +void BuildDBOptions(const ImmutableDBOptions& immutable_db_options, + const MutableDBOptions& mutable_db_options, + DBOptions& options); ColumnFamilyOptions BuildColumnFamilyOptions( const ColumnFamilyOptions& ioptions, diff --git a/options/options_settable_test.cc b/options/options_settable_test.cc index 67aab055e1..80021444fc 100644 --- a/options/options_settable_test.cc +++ b/options/options_settable_test.cc @@ -271,6 +271,12 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) { ASSERT_GT(unset_bytes_base, 0); options->~DBOptions(); + // Now also check that BuildDBOptions populates everything + FillWithSpecialChar(options_ptr, sizeof(DBOptions), kDBOptionsExcluded); + BuildDBOptions({}, {}, *options); + ASSERT_EQ(unset_bytes_base, + NumUnsetBytes(options_ptr, sizeof(DBOptions), kDBOptionsExcluded)); + options = new (options_ptr) DBOptions(); FillWithSpecialChar(options_ptr, sizeof(DBOptions), kDBOptionsExcluded); @@ -372,7 +378,11 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) { "follower_catchup_retry_count=456;" "follower_catchup_retry_wait_ms=789;" "metadata_write_temperature=kCold;" - "wal_write_temperature=kHot;", + "wal_write_temperature=kHot;" + "background_close_inactive_wals=true;" + "write_dbid_to_manifest=true;" + "write_identity_file=true;" + "prefix_seek_opt_in_only=true;", new_options)); ASSERT_EQ(unset_bytes_base, NumUnsetBytes(new_options_ptr, sizeof(DBOptions), diff --git a/unreleased_history/bug_fixes/build_db_options.md b/unreleased_history/bug_fixes/build_db_options.md new file mode 100644 index 0000000000..6994ea7193 --- /dev/null +++ b/unreleased_history/bug_fixes/build_db_options.md @@ -0,0 +1 @@ +* Several DB option settings could be lost through `GetOptionsFromString()`, possibly elsewhere as well. Affected options, now fixed:`background_close_inactive_wals`, `write_dbid_to_manifest`, `write_identity_file`, `prefix_seek_opt_in_only`