diff options
author | Peter Dillinger <peterd@meta.com> | 2024-09-26 14:36:29 -0700 |
---|---|---|
committer | Facebook GitHub Bot <facebook-github-bot@users.noreply.github.com> | 2024-09-26 14:36:29 -0700 |
commit | 79790cf2a80fb5e5b6799ebd69d3fb2ebe71d612 (patch) | |
tree | 894ce57bd4fbd5fbd10fc974e3ab1f8d430ed6bb | |
parent | 22105366d93ce3e6290282eec60529cde2f3e8d3 (diff) |
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
-rw-r--r-- | options/options_helper.cc | 19 | ||||
-rw-r--r-- | options/options_helper.h | 4 | ||||
-rw-r--r-- | options/options_settable_test.cc | 12 | ||||
-rw-r--r-- | unreleased_history/bug_fixes/build_db_options.md | 1 |
4 files changed, 31 insertions, 5 deletions
diff --git a/options/options_helper.cc b/options/options_helper.cc index 011f47b98..232b3f3bd 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 679a1a7ee..7519b627d 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 67aab055e..80021444f 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 000000000..6994ea719 --- /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` |