Fix FIFO dynamic options sanitization (#5367)

Summary:
When dynamically setting options, we check the option type info and skip options that are marked deprecated. However this check is only done at top level, which results in bugs where SetOptions will corrupt option values and cause unexpected system behavior iff a deprecated second level option is set dynamically.
For exmaple, the following call:
```
dbfull()->SetOptions(
    {{"compaction_options_fifo",
        "{allow_compaction=true;max_table_files_size=1024;ttl=731;}"}});
```
was from pre 6.0 release when `ttl` was part of `compaction_options_fifo`. Now that it got moved out of `compaction_options_fifo`, this call will incorrectly set `compaction_options_fifo.max_table_files_size` to 731 (as `max_table_files_size` is the first one in `OptionsHelper::fifo_compaction_options_type_info` struct) and cause files to gett evicted much faster than expected.

This PR adds verification to second level options like `compaction_options_fifo.ttl` or `compaction_options_fifo.max_table_files_size` when set dynamically, and filter out those marked as deprecated.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5367

Differential Revision: D15530998

Pulled By: miasantreble

fbshipit-source-id: 818258be5c3abe09cd82d62f3c083572d70fecdd
main
Zhongyi Xie 6 years ago committed by Facebook Github Bot
parent 545d206040
commit 87fe4bcab8
  1. 47
      db/db_options_test.cc
  2. 5
      options/options_helper.cc

@ -1007,6 +1007,53 @@ TEST_F(DBOptionsTest, CompactionReadaheadSizeChange) {
ASSERT_EQ(256, env_->compaction_readahead_size_); ASSERT_EQ(256, env_->compaction_readahead_size_);
Close(); Close();
} }
TEST_F(DBOptionsTest, FIFOTtlBackwardCompatible) {
Options options;
options.compaction_style = kCompactionStyleFIFO;
options.write_buffer_size = 10 << 10; // 10KB
options.create_if_missing = true;
ASSERT_OK(TryReopen(options));
Random rnd(301);
for (int i = 0; i < 10; i++) {
// Generate and flush a file about 10KB.
for (int j = 0; j < 10; j++) {
ASSERT_OK(Put(ToString(i * 20 + j), RandomString(&rnd, 980)));
}
Flush();
}
ASSERT_OK(dbfull()->TEST_WaitForCompact());
ASSERT_EQ(NumTableFilesAtLevel(0), 10);
// In release 6.0, ttl was promoted from a secondary level option under
// compaction_options_fifo to a top level option under ColumnFamilyOptions.
// We still need to handle old SetOptions calls but should ignore
// ttl under compaction_options_fifo.
ASSERT_OK(dbfull()->SetOptions(
{{"compaction_options_fifo",
"{allow_compaction=true;max_table_files_size=1024;ttl=731;}"},
{"ttl", "60"}}));
ASSERT_EQ(dbfull()->GetOptions().compaction_options_fifo.allow_compaction,
true);
ASSERT_EQ(dbfull()->GetOptions().compaction_options_fifo.max_table_files_size,
1024);
ASSERT_EQ(dbfull()->GetOptions().ttl, 60);
// Put ttl as the first option inside compaction_options_fifo. That works as
// it doesn't overwrite any other option.
ASSERT_OK(dbfull()->SetOptions(
{{"compaction_options_fifo",
"{ttl=985;allow_compaction=true;max_table_files_size=1024;}"},
{"ttl", "191"}}));
ASSERT_EQ(dbfull()->GetOptions().compaction_options_fifo.allow_compaction,
true);
ASSERT_EQ(dbfull()->GetOptions().compaction_options_fifo.max_table_files_size,
1024);
ASSERT_EQ(dbfull()->GetOptions().ttl, 191);
}
#endif // ROCKSDB_LITE #endif // ROCKSDB_LITE
} // namespace rocksdb } // namespace rocksdb

@ -372,6 +372,11 @@ bool ParseSingleStructOption(
return false; return false;
} }
const auto& opt_info = iter->second; const auto& opt_info = iter->second;
if (opt_info.verification == OptionVerificationType::kDeprecated) {
// Should also skip deprecated sub-options such as
// fifo_compaction_options_type_info.ttl
return true;
}
return ParseOptionHelper( return ParseOptionHelper(
reinterpret_cast<char*>(options) + opt_info.mutable_offset, opt_info.type, reinterpret_cast<char*>(options) + opt_info.mutable_offset, opt_info.type,
value); value);

Loading…
Cancel
Save