From 2853f79f966e1caefcfa0226efc8833b63fcab01 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Thu, 21 Dec 2023 16:19:42 +0200 Subject: [PATCH 1/2] virtual_tables: scope virtual tables registry in system_keyspace Virtual tables are kept in a thread_local registry for deduplication purposes. The problem is that thread_local variables are destroyed late, possibly after the schema registry and the reactor are destroyed. Currently this isn't a problem, but after a seastar change to destroy the reactor after termination [1], things break. Fix by moving the registry to system_keyspace. system_keyspace was chosen since it was the birthplace of virtual tables. Pimpl is used to avoid increasing dependencies. [1] https://github.com/scylladb/seastar/commit/101b245ed709d7170f3addcb4ebdc87763d6f65b --- db/system_keyspace.hh | 3 +++ db/virtual_tables.cc | 17 +++++++++++------ db/virtual_tables.hh | 13 +++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/db/system_keyspace.hh b/db/system_keyspace.hh index 34d25812d9..b9543b1d85 100644 --- a/db/system_keyspace.hh +++ b/db/system_keyspace.hh @@ -24,6 +24,7 @@ #include "cdc/generation_id.hh" #include "locator/host_id.hh" #include "mutation/canonical_mutation.hh" +#include "virtual_tables.hh" namespace sstables { struct entry_descriptor; @@ -112,6 +113,7 @@ class system_keyspace : public seastar::peering_sharded_service cql3::query_processor& _qp; replica::database& _db; std::unique_ptr _cache; + virtual_tables_registry _virtual_tables_registry; static schema_ptr raft_snapshot_config(); static schema_ptr local(); @@ -521,6 +523,7 @@ public: ~system_keyspace(); future<> shutdown(); + virtual_tables_registry& get_virtual_tables_registry() { return _virtual_tables_registry; } private: future<::shared_ptr> execute_cql(const sstring& query_string, const std::initializer_list& values); template diff --git a/db/virtual_tables.cc b/db/virtual_tables.cc index a1635148fd..193ee8f3d2 100644 --- a/db/virtual_tables.cc +++ b/db/virtual_tables.cc @@ -982,11 +982,8 @@ private: } -// Map from table's schema ID to table itself. Helps avoiding accidental duplication. -static thread_local std::map> virtual_tables; - -static void register_virtual_tables(distributed& dist_db, distributed& dist_ss, sharded& dist_gossiper, sharded& dist_raft_gr, db::config& cfg) { - auto add_table = [] (std::unique_ptr&& tbl) { +static void register_virtual_tables(virtual_tables_registry_impl& virtual_tables, distributed& dist_db, distributed& dist_ss, sharded& dist_gossiper, sharded& dist_raft_gr, db::config& cfg) { + auto add_table = [&] (std::unique_ptr&& tbl) { virtual_tables[tbl->schema()->id()] = std::move(tbl); }; @@ -1007,6 +1004,7 @@ static void register_virtual_tables(distributed& dist_db, dis } static void install_virtual_readers_and_writers(db::system_keyspace& sys_ks, replica::database& db) { + auto& virtual_tables = *sys_ks.get_virtual_tables_registry(); db.find_column_family(system_keyspace::size_estimates()).set_virtual_reader(mutation_source(db::size_estimates::virtual_reader(db, sys_ks))); db.find_column_family(system_keyspace::v3::views_builds_in_progress()).set_virtual_reader(mutation_source(db::view::build_progress_virtual_reader(db))); db.find_column_family(system_keyspace::built_indexes()).set_virtual_reader(mutation_source(db::index::built_indexes_virtual_reader(db))); @@ -1023,7 +1021,9 @@ future<> initialize_virtual_tables( sharded& dist_gossiper, distributed& dist_raft_gr, sharded& sys_ks, db::config& cfg) { - register_virtual_tables(dist_db, dist_ss, dist_gossiper, dist_raft_gr, cfg); + auto& virtual_tables_registry = sys_ks.local().get_virtual_tables_registry(); + auto& virtual_tables = *virtual_tables_registry; + register_virtual_tables(virtual_tables, dist_db, dist_ss, dist_gossiper, dist_raft_gr, cfg); auto& db = dist_db.local(); for (auto&& [id, vt] : virtual_tables) { @@ -1034,4 +1034,9 @@ future<> initialize_virtual_tables( install_virtual_readers_and_writers(sys_ks.local(), db); } +virtual_tables_registry::virtual_tables_registry() : unique_ptr(std::make_unique()) { +} + +virtual_tables_registry::~virtual_tables_registry() = default; + } // namespace db diff --git a/db/virtual_tables.hh b/db/virtual_tables.hh index 4d7213af21..f03421680f 100644 --- a/db/virtual_tables.hh +++ b/db/virtual_tables.hh @@ -10,6 +10,7 @@ #pragma once #include +#include #include "schema/schema_fwd.hh" namespace replica { @@ -38,4 +39,16 @@ future<> initialize_virtual_tables( sharded& sys_ks, db::config&); + +class virtual_table; + +using virtual_tables_registry_impl = std::map>; + +// Pimpl to hide virtual_table from the rest of the code +class virtual_tables_registry : public std::unique_ptr { +public: + virtual_tables_registry(); + ~virtual_tables_registry(); +}; + } // namespace db From c00b376a3e86eea074784c199f05ab70f25b3438 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Thu, 21 Dec 2023 16:24:02 +0200 Subject: [PATCH 2/2] schema_registry, database: flush entries when no longer in use The schema registry disarms internal timers when it is destroyed. This accesses the Seastar reactor. However, after [1] we don't have ordering between the reactor destruction and the thread_local registry destruction. Fix this by flushing all entries when the database is destroyed. The database object is fundamental so it's unlikely we'll have anything using the registry after it's gone. [1] https://github.com/scylladb/seastar/commit/101b245ed709d7170f3addcb4ebdc87763d6f65b --- replica/database.cc | 1 + schema/schema_registry.cc | 4 ++++ schema/schema_registry.hh | 6 ++++++ 3 files changed, 11 insertions(+) diff --git a/replica/database.cc b/replica/database.cc index b22a8e8d26..2c01a8e69c 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -636,6 +636,7 @@ void database::set_format(sstables::sstable_version_types format) noexcept { database::~database() { _user_types->deactivate(); + local_schema_registry().clear(); } void database::update_version(const table_schema_version& version) { diff --git a/schema/schema_registry.cc b/schema/schema_registry.cc index 2fe0b1cf02..c2d462649f 100644 --- a/schema/schema_registry.cc +++ b/schema/schema_registry.cc @@ -164,6 +164,10 @@ schema_ptr schema_registry::get_or_load(table_schema_version v, const schema_loa return e.get_schema(); } +void schema_registry::clear() { + _entries.clear(); +} + schema_ptr schema_registry_entry::load(frozen_schema fs) { _frozen_schema = std::move(fs); auto s = get_schema(); diff --git a/schema/schema_registry.hh b/schema/schema_registry.hh index 8f1b95e04d..9490439021 100644 --- a/schema/schema_registry.hh +++ b/schema/schema_registry.hh @@ -150,6 +150,12 @@ public: // The schema instance pointed to by the argument will be attached to the registry // entry and will keep it alive. schema_ptr learn(const schema_ptr&); + + // Removes all entries from the registry. This in turn removes all dependencies + // on the Seastar reactor. + // + // Prerequisite: all futures from get_or_load() are resolved. + void clear(); }; schema_registry& local_schema_registry();