Skip to content

Commit

Permalink
Make filter_policy a mutable option, fix race
Browse files Browse the repository at this point in the history
Summary: I wanted to make filter_policy a mutable option, but the
SetOptions race fix proposed in facebook#10480 doesn't work for object options
like filter_policy. (Race still reported.)

I found that when the options objects are copied for persisting to file,
the table factory (and thus options) is not copied, therefore leading to
a race between persisting the options and subsequent updates with
SetOptions(). It appears that fixing this fixes the race that was being
reported with SetOptions() in the

Fixes facebook#10079

Intended follow-up: See facebook#10021

Test Plan: unit test added, crash test updated with more SetOptions and
higher probability of calling it.

Ran TSAN blackbox crashtest for hours with options to amplify potential
race (see facebook#10079)
  • Loading branch information
pdillinger committed Sep 12, 2023
1 parent 4b123f3 commit 33b989a
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 3 deletions.
51 changes: 51 additions & 0 deletions db/db_bloom_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1775,6 +1775,57 @@ TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) {
}
}

TEST_F(DBBloomFilterTest, MutableFilterPolicy) {
// Test that BlockBasedTableOptions::filter_policy is mutable with
// SetOptions.

Options options = CurrentOptions();
options.statistics = CreateDBStatistics();
auto& stats = *options.statistics;
BlockBasedTableOptions table_options;
// First config, to make sure there's no issues with this shared ptr
// etc. when the DB switches filter policies.
table_options.filter_policy.reset(NewBloomFilterPolicy(10));
double expected_bpk = 10.0;
// Other configs to try
std::vector<std::pair<std::string, double>> configs = {
{"ribbonfilter:10:-1", 7.0}, {"bloomfilter:5", 5.0}};

table_options.cache_index_and_filter_blocks = true;
options.table_factory.reset(NewBlockBasedTableFactory(table_options));

ASSERT_OK(TryReopen(options));

char v[] = "a";

for (;; ++(v[0])) {
const int maxKey = 8000;
for (int i = 0; i < maxKey; i++) {
ASSERT_OK(Put(Key(i), v));
}
ASSERT_OK(Flush());

for (int i = 0; i < maxKey; i++) {
ASSERT_EQ(Get(Key(i)), v);
}

uint64_t filter_bytes =
stats.getAndResetTickerCount(BLOCK_CACHE_FILTER_BYTES_INSERT);

EXPECT_NEAR(filter_bytes * 8.0 / maxKey, expected_bpk, 0.3);

if (configs.empty()) {
break;
}

ASSERT_OK(
db_->SetOptions({{"block_based_table_factory",
"{filter_policy=" + configs.back().first + ";}"}}));
expected_bpk = configs.back().second;
configs.pop_back();
}
}

class SliceTransformLimitedDomain : public SliceTransform {
const char* Name() const override { return "SliceTransformLimitedDomain"; }

Expand Down
3 changes: 3 additions & 0 deletions db_stress_tool/db_stress_test_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ bool StressTest::BuildOptionsTable() {
"2",
}},
{"max_sequential_skip_in_iterations", {"4", "8", "12"}},
{"block_based_table_factory",
{"{filter_policy=bloomfilter:10}", "{filter_policy=ribbonfilter:10}"}},
{"block_based_table_factory", {"{block_size=2048}", "{block_size=4096}"}},
};

if (FLAGS_allow_setting_blob_options_dynamically) {
Expand Down
6 changes: 6 additions & 0 deletions options/options_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ ColumnFamilyOptions BuildColumnFamilyOptions(
UpdateColumnFamilyOptions(mutable_cf_options, &cf_opts);
// TODO(yhchiang): find some way to handle the following derived options
// * max_file_size
if (cf_opts.table_factory->IsInstanceOf(
TableFactory::kBlockBasedTableName())) {
// Deep copy the factory and its options, because they are mutable
cf_opts.table_factory.reset(NewBlockBasedTableFactory(
*cf_opts.table_factory->GetOptions<BlockBasedTableOptions>()));
}
return cf_opts;
}

Expand Down
3 changes: 1 addition & 2 deletions table/block_based/block_based_table_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ static std::unordered_map<std::string,
{"kFlushOnly",
BlockBasedTableOptions::PrepopulateBlockCache::kFlushOnly}};


static std::unordered_map<std::string, OptionTypeInfo>
block_based_table_type_info = {
/* currently not supported
Expand Down Expand Up @@ -314,7 +313,7 @@ static std::unordered_map<std::string, OptionTypeInfo>
OptionTypeInfo::AsCustomSharedPtr<const FilterPolicy>(
offsetof(struct BlockBasedTableOptions, filter_policy),
OptionVerificationType::kByNameAllowFromNull,
OptionTypeFlags::kNone)},
(OptionTypeFlags::kMutable | OptionTypeFlags::kAllowNull))},
{"whole_key_filtering",
{offsetof(struct BlockBasedTableOptions, whole_key_filtering),
OptionType::kBoolean, OptionVerificationType::kNormal,
Expand Down
2 changes: 1 addition & 1 deletion tools/db_crashtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ def is_direct_io_supported(dbname):
# since we will be killing anyway, use large value for ops_per_thread
"ops_per_thread": 100000000,
"reopen": 0,
"set_options_one_in": 10000,
"set_options_one_in": 1000,
}

whitebox_default_params = {
Expand Down
1 change: 1 addition & 0 deletions unreleased_history/bug_fixes/set_options_race
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a rare and probably not dangerous race condition in SetOptions for block_based_table_factory options.

0 comments on commit 33b989a

Please sign in to comment.