From aa667e590e9630a1f16b8c8a2a0894498a7f2faa Mon Sep 17 00:00:00 2001 From: "Raphael S. Carvalho" Date: Fri, 17 Jun 2022 17:34:38 -0300 Subject: [PATCH] sstable_set: Fix partitioned_sstable_set constructor The sstable set param isn't being used anywhere, and it's also buggy as sstable run list isn't being updated accordingly. so it could happen that set contains sstables but run list is empty, introducing inconsistency. we're fortunate that the bug wasn't activated as it would've been a hard one to catch. found this while auditting the code. Signed-off-by: Raphael S. Carvalho Message-Id: <20220617203438.74336-1-raphaelsc@scylladb.com> --- compaction/compaction.cc | 4 ++-- db/view/view_update_generator.cc | 2 +- replica/table.cc | 2 +- sstables/sstable_set.cc | 12 ++++++------ sstables/sstable_set.hh | 2 +- sstables/sstable_set_impl.hh | 2 +- sstables_loader.cc | 3 +-- test/boost/sstable_compaction_test.cc | 4 ++-- test/boost/sstable_set_test.cc | 6 +++++- 9 files changed, 20 insertions(+), 17 deletions(-) diff --git a/compaction/compaction.cc b/compaction/compaction.cc index 01915c65eb..4dbc507bc9 100644 --- a/compaction/compaction.cc +++ b/compaction/compaction.cc @@ -897,7 +897,7 @@ public: } virtual sstables::sstable_set make_sstable_set_for_input() const override { - return sstables::make_partitioned_sstable_set(_schema, make_lw_shared(sstable_list{}), false); + return sstables::make_partitioned_sstable_set(_schema, false); } flat_mutation_reader_v2 make_sstable_reader() const override { @@ -1692,7 +1692,7 @@ static future scrub_sstables_validate_mode(sstables::compacti auto schema = table_s.schema(); formatted_sstables_list sstables_list_msg; - auto sstables = make_lw_shared(sstables::make_partitioned_sstable_set(schema, make_lw_shared(sstable_list{}), false)); + auto sstables = make_lw_shared(sstables::make_partitioned_sstable_set(schema, false)); for (const auto& sst : descriptor.sstables) { sstables_list_msg += sst; sstables->insert(sst); diff --git a/db/view/view_update_generator.cc b/db/view/view_update_generator.cc index b8ceddde79..7f6946def3 100644 --- a/db/view/view_update_generator.cc +++ b/db/view/view_update_generator.cc @@ -58,7 +58,7 @@ future<> view_update_generator::start() { // Exploit the fact that sstables in the staging directory // are usually non-overlapping and use a partitioned set for // the read. - auto ssts = make_lw_shared(sstables::make_partitioned_sstable_set(s, make_lw_shared(sstable_list{}), false)); + auto ssts = make_lw_shared(sstables::make_partitioned_sstable_set(s, false)); for (auto& sst : sstables) { ssts->insert(sst); } diff --git a/replica/table.cc b/replica/table.cc index bfeac7e394..a80079be02 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -97,7 +97,7 @@ lw_shared_ptr table::make_maintenance_sstable_set() const // Level metadata is not used because (level 0) maintenance sstables are disjoint and must be stored for efficient retrieval in the partitioned set bool use_level_metadata = false; return make_lw_shared( - sstables::make_partitioned_sstable_set(_schema, make_lw_shared(sstable_list{}), use_level_metadata)); + sstables::make_partitioned_sstable_set(_schema, use_level_metadata)); } void table::refresh_compound_sstable_set() { diff --git a/sstables/sstable_set.cc b/sstables/sstable_set.cc index eb906854f7..a19d9fb9da 100644 --- a/sstables/sstable_set.cc +++ b/sstables/sstable_set.cc @@ -224,9 +224,9 @@ dht::partition_range partitioned_sstable_set::to_partition_range(const dht::ring return dht::partition_range::make(std::move(lower_bound), std::move(upper_bound)); } -partitioned_sstable_set::partitioned_sstable_set(schema_ptr schema, lw_shared_ptr all, bool use_level_metadata) +partitioned_sstable_set::partitioned_sstable_set(schema_ptr schema, bool use_level_metadata) : _schema(std::move(schema)) - , _all(std::move(all)) + , _all(make_lw_shared()) , _use_level_metadata(use_level_metadata) { } @@ -589,19 +589,19 @@ std::unique_ptr partitioned_sstable_set::make_increme std::unique_ptr compaction_strategy_impl::make_sstable_set(schema_ptr schema) const { // with use_level_metadata enabled, L0 sstables will not go to interval map, which suits well STCS. - return std::make_unique(schema, make_lw_shared(), true); + return std::make_unique(schema, true); } std::unique_ptr leveled_compaction_strategy::make_sstable_set(schema_ptr schema) const { - return std::make_unique(std::move(schema), make_lw_shared()); + return std::make_unique(std::move(schema)); } std::unique_ptr time_window_compaction_strategy::make_sstable_set(schema_ptr schema) const { return std::make_unique(std::move(schema)); } -sstable_set make_partitioned_sstable_set(schema_ptr schema, lw_shared_ptr all, bool use_level_metadata) { - return sstable_set(std::make_unique(schema, std::move(all), use_level_metadata), schema); +sstable_set make_partitioned_sstable_set(schema_ptr schema, bool use_level_metadata) { + return sstable_set(std::make_unique(schema, use_level_metadata), schema); } sstable_set diff --git a/sstables/sstable_set.hh b/sstables/sstable_set.hh index dcc1c5ce3f..cd1b20a0dd 100644 --- a/sstables/sstable_set.hh +++ b/sstables/sstable_set.hh @@ -144,7 +144,7 @@ public: friend class compound_sstable_set; }; -sstable_set make_partitioned_sstable_set(schema_ptr schema, lw_shared_ptr all, bool use_level_metadata = true); +sstable_set make_partitioned_sstable_set(schema_ptr schema, bool use_level_metadata = true); sstable_set make_compound_sstable_set(schema_ptr schema, std::vector> sets); diff --git a/sstables/sstable_set_impl.hh b/sstables/sstable_set_impl.hh index a65efd06f3..8446971fe0 100644 --- a/sstables/sstable_set_impl.hh +++ b/sstables/sstable_set_impl.hh @@ -79,7 +79,7 @@ public: static dht::partition_range to_partition_range(const dht::ring_position_view& pos, const interval_type& i); partitioned_sstable_set(const partitioned_sstable_set&) = delete; - explicit partitioned_sstable_set(schema_ptr schema, lw_shared_ptr all, bool use_level_metadata = true); + explicit partitioned_sstable_set(schema_ptr schema, bool use_level_metadata = true); // For cloning the partitioned_sstable_set (makes a deep copy, including *_all) explicit partitioned_sstable_set( schema_ptr schema, diff --git a/sstables_loader.cc b/sstables_loader.cc index a8e96dd717..a77ea91faa 100644 --- a/sstables_loader.cc +++ b/sstables_loader.cc @@ -128,8 +128,7 @@ future<> sstables_loader::load_and_stream(sstring ks_name, sstring cf_name, size_t nr_sst_current = 0; while (!sstables.empty()) { auto ops_uuid = utils::make_random_uuid(); - auto sst_set = make_lw_shared(sstables::make_partitioned_sstable_set(s, - make_lw_shared(sstable_list{}), false)); + auto sst_set = make_lw_shared(sstables::make_partitioned_sstable_set(s, false)); size_t batch_sst_nr = 16; std::vector sst_names; std::vector sst_processed; diff --git a/test/boost/sstable_compaction_test.cc b/test/boost/sstable_compaction_test.cc index 440f3c7699..3cd4d505e0 100644 --- a/test/boost/sstable_compaction_test.cc +++ b/test/boost/sstable_compaction_test.cc @@ -4654,8 +4654,8 @@ SEASTAR_TEST_CASE(compound_sstable_set_incremental_selector_test) { }; auto incremental_selection_test = [&] (strategy_param param) { - auto set1 = make_lw_shared(sstables::make_partitioned_sstable_set(s, make_lw_shared(), false)); - auto set2 = make_lw_shared(sstables::make_partitioned_sstable_set(s, make_lw_shared(), bool(param))); + auto set1 = make_lw_shared(sstables::make_partitioned_sstable_set(s, false)); + auto set2 = make_lw_shared(sstables::make_partitioned_sstable_set(s, bool(param))); set1->insert(sstable_for_overlapping_test(env, s, 0, key_and_token_pair[1].first, key_and_token_pair[1].first, 1)); set2->insert(sstable_for_overlapping_test(env, s, 1, key_and_token_pair[0].first, key_and_token_pair[2].first, 1)); set2->insert(sstable_for_overlapping_test(env, s, 2, key_and_token_pair[3].first, key_and_token_pair[3].first, 1)); diff --git a/test/boost/sstable_set_test.cc b/test/boost/sstable_set_test.cc index 727fed84ff..ac2e280807 100644 --- a/test/boost/sstable_set_test.cc +++ b/test/boost/sstable_set_test.cc @@ -16,7 +16,11 @@ #include "readers/from_mutations_v2.hh" static sstables::sstable_set make_sstable_set(schema_ptr schema, lw_shared_ptr all = {}, bool use_level_metadata = true) { - return sstables::sstable_set(std::make_unique(schema, std::move(all), use_level_metadata), schema); + auto ret = sstables::sstable_set(std::make_unique(schema, use_level_metadata), schema); + for (auto& sst : *all) { + ret.insert(sst); + } + return ret; } SEASTAR_TEST_CASE(test_sstables_sstable_set_read_modify_write) {