From 9bfe9f0b2f76f394378c93fdca66edc9150b04e5 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 29 Jun 2023 01:10:58 +0200 Subject: [PATCH 1/4] schema_tables: Avoid crashing when table selector has only one kind of tables Currently not reachable, because selectors are always constructed with both kinds initailized. Will be triggered by the next patch. --- db/schema_tables.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 00a234036faa..490778b9069a 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -1067,6 +1067,9 @@ read_tables_for_keyspaces(distributed& proxy, const std: { std::map result; for (auto&& [keyspace_name, sel] : tables_per_keyspace) { + if (!sel.tables.contains(kind)) { + continue; + } for (auto&& table_name : sel.tables.find(kind)->second) { auto qn = qualified_name(keyspace_name, table_name); auto muts = co_await read_table_mutations(proxy, qn, get_table_holder(kind)); From 0c86abab4d21e53a61a2a4216d5556574413ea96 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 29 Jun 2023 01:08:51 +0200 Subject: [PATCH 2/4] migration_manager, schema_tables: Implement migration_manager::reload_schema() Will recreate schema_ptr's from schema tables like during table alter. Will be needed when digest calculation changes in reaction to cluster feature at run time. --- db/schema_tables.cc | 37 +++++++++++++++++++++++++++--------- db/schema_tables.hh | 2 +- service/migration_manager.cc | 6 ++++++ service/migration_manager.hh | 1 + 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 490778b9069a..02431f749801 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -151,7 +151,8 @@ static future<> merge_tables_and_views(distributed& prox std::map&& tables_before, std::map&& tables_after, std::map&& views_before, - std::map&& views_after); + std::map&& views_after, + bool reload); struct [[nodiscard]] user_types_to_drop final { seastar::noncopyable_function ()> drop; @@ -164,7 +165,7 @@ static future merge_types(distributed merge_functions(distributed& proxy, schema_result before, schema_result after); static future<> merge_aggregates(distributed& proxy, schema_result before, schema_result after, schema_result scylla_before, schema_result scylla_after); -static future<> do_merge_schema(distributed&, std::vector, bool do_flush); +static future<> do_merge_schema(distributed&, std::vector, bool do_flush, bool reload); using computed_columns_map = std::unordered_map; static computed_columns_map get_computed_columns(const schema_mutations& sm); @@ -969,7 +970,7 @@ future<> update_schema_version_and_announce(sharded& sys_ks * @throws ConfigurationException If one of metadata attributes has invalid value * @throws IOException If data was corrupted during transportation or failed to apply fs operations */ -future<> merge_schema(sharded& sys_ks, distributed& proxy, gms::feature_service& feat, std::vector mutations) +future<> merge_schema(sharded& sys_ks, distributed& proxy, gms::feature_service& feat, std::vector mutations, bool reload) { if (this_shard_id() != 0) { // mutations must be applied on the owning shard (0). @@ -980,7 +981,7 @@ future<> merge_schema(sharded& sys_ks, distributed future<> { bool flush_schema = proxy.local().get_db().local().get_config().flush_schema_tables_after_modification(); - co_await do_merge_schema(proxy, std::move(mutations), flush_schema); + co_await do_merge_schema(proxy, std::move(mutations), flush_schema, reload); co_await update_schema_version_and_announce(sys_ks, proxy, feat.cluster_schema_features()); }); } @@ -1222,7 +1223,7 @@ table_selector get_affected_tables(const sstring& keyspace_name, const mutation& return result; } -static future<> do_merge_schema(distributed& proxy, std::vector mutations, bool do_flush) +static future<> do_merge_schema(distributed& proxy, std::vector mutations, bool do_flush, bool reload) { slogger.trace("do_merge_schema: {}", mutations); schema_ptr s = keyspaces(); @@ -1249,6 +1250,15 @@ static future<> do_merge_schema(distributed& proxy, std: delete_schema_version(mutation); } + if (reload) { + for (auto&& ks : proxy.local().get_db().local().get_non_system_keyspaces()) { + keyspaces.emplace(ks); + table_selector sel; + sel.all_in_keyspace = true; + affected_tables[ks] = sel; + } + } + // Resolve sel.all_in_keyspace == true to the actual list of tables and views. for (auto&& [keyspace_name, sel] : affected_tables) { if (sel.all_in_keyspace) { @@ -1305,7 +1315,7 @@ static future<> do_merge_schema(distributed& proxy, std: auto types_to_drop = co_await merge_types(proxy, std::move(old_types), std::move(new_types)); co_await merge_tables_and_views(proxy, std::move(old_column_families), std::move(new_column_families), - std::move(old_views), std::move(new_views)); + std::move(old_views), std::move(new_views), reload); co_await merge_functions(proxy, std::move(old_functions), std::move(new_functions)); co_await merge_aggregates(proxy, std::move(old_aggregates), std::move(new_aggregates), std::move(old_scylla_aggregates), std::move(new_scylla_aggregates)); co_await types_to_drop.drop(); @@ -1409,6 +1419,7 @@ enum class schema_diff_side { static schema_diff diff_table_or_view(distributed& proxy, std::map&& before, std::map&& after, + bool reload, noncopyable_function create_schema) { schema_diff d; @@ -1429,6 +1440,13 @@ static schema_diff diff_table_or_view(distributed& proxy slogger.info("Altering {}.{} id={} version={}", s->ks_name(), s->cf_name(), s->id(), s->version()); d.altered.emplace_back(schema_diff::altered_schema{s_before, s}); } + if (reload) { + for (auto&& key: diff.entries_in_common) { + auto s = create_schema(std::move(after.at(key)), schema_diff_side::right); + slogger.info("Reloading {}.{} id={} version={}", s->ks_name(), s->cf_name(), s->id(), s->version()); + d.altered.emplace_back(schema_diff::altered_schema {s, s}); + } + } return d; } @@ -1441,12 +1459,13 @@ static future<> merge_tables_and_views(distributed& prox std::map&& tables_before, std::map&& tables_after, std::map&& views_before, - std::map&& views_after) + std::map&& views_after, + bool reload) { - auto tables_diff = diff_table_or_view(proxy, std::move(tables_before), std::move(tables_after), [&] (schema_mutations sm, schema_diff_side) { + auto tables_diff = diff_table_or_view(proxy, std::move(tables_before), std::move(tables_after), reload, [&] (schema_mutations sm, schema_diff_side) { return create_table_from_mutations(proxy, std::move(sm)); }); - auto views_diff = diff_table_or_view(proxy, std::move(views_before), std::move(views_after), [&] (schema_mutations sm, schema_diff_side side) { + auto views_diff = diff_table_or_view(proxy, std::move(views_before), std::move(views_after), reload, [&] (schema_mutations sm, schema_diff_side side) { // The view schema mutation should be created with reference to the base table schema because we definitely know it by now. // If we don't do it we are leaving a window where write commands to this schema are illegal. // There are 3 possibilities: diff --git a/db/schema_tables.hh b/db/schema_tables.hh index 83c564f39be3..f650f83bf75b 100644 --- a/db/schema_tables.hh +++ b/db/schema_tables.hh @@ -189,7 +189,7 @@ future read_keyspace_mutation(distributed&, co // Must be called on shard 0. future> hold_merge_lock() noexcept; -future<> merge_schema(sharded& sys_ks, distributed& proxy, gms::feature_service& feat, std::vector mutations); +future<> merge_schema(sharded& sys_ks, distributed& proxy, gms::feature_service& feat, std::vector mutations, bool reload = false); // Recalculates the local schema version. // diff --git a/service/migration_manager.cc b/service/migration_manager.cc index 818dcb6ccfd4..0e8ba3fda8b8 100644 --- a/service/migration_manager.cc +++ b/service/migration_manager.cc @@ -368,6 +368,12 @@ future<> migration_manager::merge_schema_from(netw::messaging_service::msg_addr return db::schema_tables::merge_schema(_sys_ks, proxy.container(), _feat, std::move(mutations)); } +future<> migration_manager::reload_schema() { + mlogger.info("Reloading schema"); + std::vector mutations; + return db::schema_tables::merge_schema(_sys_ks, _storage_proxy.container(), _feat, std::move(mutations), true); +} + future<> migration_manager::merge_schema_from(netw::messaging_service::msg_addr src, const std::vector& mutations) { if (_as.abort_requested()) { diff --git a/service/migration_manager.hh b/service/migration_manager.hh index bde41921451f..352ac35755e1 100644 --- a/service/migration_manager.hh +++ b/service/migration_manager.hh @@ -94,6 +94,7 @@ public: // Coalesces requests. future<> merge_schema_from(netw::msg_addr); future<> do_merge_schema_from(netw::msg_addr); + future<> reload_schema(); // Merge mutations received from src. // Keep mutations alive around whole async operation. From f2ed9fcd7e95012ca1af7c49f0afbf33d092eef2 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 29 Jun 2023 01:12:35 +0200 Subject: [PATCH 3/4] schema_mutations, migration_manager: Ignore empty partitions in per-table digest Schema digest is calculated by querying for mutations of all schema tables, then compacting them so that all tombstones in them are dropped. However, even if the mutation becomes empty after compaction, we still feed its partition key. If the same mutations were compacted prior to the query, because the tombstones expire, we won't get any mutation at all and won't feed the partition key. So schema digest will change once an empty partition of some schema table is compacted away. Tombstones expire 7 days after schema change which introduces them. If one of the nodes is restarted after that, it will compute a different table schema digest on boot. This may cause performance problems. When sending a request from coordinator to replica, the replica needs schema_ptr of exact schema version request by the coordinator. If it doesn't know that version, it will request it from the coordinator and perform a full schema merge. This adds latency to every such request. Schema versions which are not referenced are currently kept in cache for only 1 second, so if request flow has low-enough rate, this situation results in perpetual schema pulls. After ae8d2a550d227de2221c310bd38704ad2e078b35, it is more liekly to run into this situation, because table creation generates tombstones for all schema tables relevant to the table, even the ones which will be otherwise empty for the new table (e.g. computed_columns). This change inroduces a cluster feature which when enabled will change digest calculation to be insensitive to expiry by ignoring empty partitions in digest calculation. When the feature is enabled, schema_ptrs are reloaded so that the window of discrepancy during transition is short and no rolling restart is required. A similar problem was fixed for per-node digest calculation in 18f484cc753d17d1e3658bcb5c73ed8f319d32e8. Per-table digest calculation was not fixed at that time because we didn't persist enabled features and they were not enabled early-enough on boot for us to depend on them in digest calculation. Now they are enabled before non-system tables are loaded so digest calculation can rely on cluster features. Fixes #4485. --- db/config.cc | 2 ++ db/config.hh | 1 + db/schema_features.hh | 7 ++++++- db/schema_tables.cc | 10 ++++++---- db/schema_tables.hh | 11 +++++++++-- gms/feature_service.cc | 4 ++++ gms/feature_service.hh | 1 + schema_mutations.cc | 25 ++++++++++++++----------- schema_mutations.hh | 3 ++- service/migration_manager.cc | 8 ++++++++ test/boost/schema_change_test.cc | 13 ++++++------- test/boost/schema_registry_test.cc | 8 +++++--- tools/schema_loader.cc | 4 +++- 13 files changed, 67 insertions(+), 30 deletions(-) diff --git a/db/config.cc b/db/config.cc index dfce1310e541..19e5e1ccbb1a 100644 --- a/db/config.cc +++ b/db/config.cc @@ -881,6 +881,8 @@ db::config::config(std::shared_ptr exts) , uuid_sstable_identifiers_enabled(this, "uuid_sstable_identifiers_enabled", liveness::LiveUpdate, value_status::Used, true, "If set to true, each newly created sstable will have a UUID " "based generation identifier, and such files are not readable by previous Scylla versions.") + , table_digest_insensitive_to_expiry(this, "table_digest_insensitive_to_expiry", liveness::MustRestart, value_status::Used, true, + "When enabled, per-table schema digest calculation ignores empty partitions.") , enable_dangerous_direct_import_of_cassandra_counters(this, "enable_dangerous_direct_import_of_cassandra_counters", value_status::Used, false, "Only turn this option on if you want to import tables from Cassandra containing counters, and you are SURE that no counters in that table were created in a version earlier than Cassandra 2.1." " It is not enough to have ever since upgraded to newer versions of Cassandra. If you EVER used a version earlier than 2.1 in the cluster where these SSTables come from, DO NOT TURN ON THIS OPTION! You will corrupt your data. You have been warned.") , enable_shard_aware_drivers(this, "enable_shard_aware_drivers", value_status::Used, true, "Enable native transport drivers to use connection-per-shard for better performance") diff --git a/db/config.hh b/db/config.hh index cf4dbb81e415..322e539d5009 100644 --- a/db/config.hh +++ b/db/config.hh @@ -357,6 +357,7 @@ public: named_value enable_sstables_md_format; named_value sstable_format; named_value uuid_sstable_identifiers_enabled; + named_value table_digest_insensitive_to_expiry; named_value enable_dangerous_direct_import_of_cassandra_counters; named_value enable_shard_aware_drivers; named_value enable_ipv6_dns_lookup; diff --git a/db/schema_features.hh b/db/schema_features.hh index c4d37f9c7236..0c48ad260d92 100644 --- a/db/schema_features.hh +++ b/db/schema_features.hh @@ -24,6 +24,10 @@ enum class schema_feature { PER_TABLE_PARTITIONERS, SCYLLA_KEYSPACES, SCYLLA_AGGREGATES, + + // When enabled, schema_mutations::digest() will skip empty mutations (with only tombstones), + // so that the digest remains the same after schema tables are compacted. + TABLE_DIGEST_INSENSITIVE_TO_EXPIRY, }; using schema_features = enum_set>; } diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 02431f749801..aee556b384f9 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -102,8 +102,10 @@ namespace { }); } -schema_ctxt::schema_ctxt(const db::config& cfg, std::shared_ptr uts, replica::database* db) +schema_ctxt::schema_ctxt(const db::config& cfg, std::shared_ptr uts, + const gms::feature_service& features, replica::database* db) : _db(db) + , _features(features) , _extensions(cfg.extensions()) , _murmur3_partitioner_ignore_msb_bits(cfg.murmur3_partitioner_ignore_msb_bits()) , _schema_registry_grace_period(cfg.schema_registry_grace_period()) @@ -111,7 +113,7 @@ schema_ctxt::schema_ctxt(const db::config& cfg, std::shared_ptr& db) @@ -3127,7 +3129,7 @@ schema_ptr create_table_from_mutations(const schema_ctxt& ctxt, schema_mutations if (version) { builder.with_version(*version); } else { - builder.with_version(sm.digest()); + builder.with_version(sm.digest(ctxt.features().cluster_schema_features())); } if (auto partitioner = sm.partitioner()) { @@ -3347,7 +3349,7 @@ view_ptr create_view_from_mutations(const schema_ctxt& ctxt, schema_mutations sm if (version) { builder.with_version(*version); } else { - builder.with_version(sm.digest()); + builder.with_version(sm.digest(ctxt.features().cluster_schema_features())); } auto base_id = table_id(row.get_nonnull("base_table_id")); diff --git a/db/schema_tables.hh b/db/schema_tables.hh index f650f83bf75b..de628558e9d2 100644 --- a/db/schema_tables.hh +++ b/db/schema_tables.hh @@ -14,6 +14,7 @@ #include "schema/schema_fwd.hh" #include "schema_features.hh" #include "utils/hashing.hh" +#include "gms/feature_service.hh" #include "schema_mutations.hh" #include "types/map.hh" #include "query-result-set.hh" @@ -66,7 +67,8 @@ class config; class schema_ctxt { public: - schema_ctxt(const config&, std::shared_ptr uts, replica::database* = nullptr); + schema_ctxt(const config&, std::shared_ptr uts, const gms::feature_service&, + replica::database* = nullptr); schema_ctxt(replica::database&); schema_ctxt(distributed&); schema_ctxt(distributed&); @@ -87,11 +89,16 @@ public: return *_user_types; } - replica::database* get_db() { + const gms::feature_service& features() const { + return _features; + } + + replica::database* get_db() const { return _db; } private: replica::database* _db; + const gms::feature_service& _features; const db::extensions& _extensions; const unsigned _murmur3_partitioner_ignore_msb_bits; const uint32_t _schema_registry_grace_period; diff --git a/gms/feature_service.cc b/gms/feature_service.cc index abd67119072a..305b43733b07 100644 --- a/gms/feature_service.cc +++ b/gms/feature_service.cc @@ -75,6 +75,9 @@ feature_config feature_config_from_db_config(const db::config& cfg, std::set(per_table_partitioners); f.set_if(keyspace_storage_options); f.set_if(aggregate_storage_options); + f.set_if(table_digest_insensitive_to_expiry); return f; } diff --git a/gms/feature_service.hh b/gms/feature_service.hh index d3ea900b65dd..ed62226d62d3 100644 --- a/gms/feature_service.hh +++ b/gms/feature_service.hh @@ -118,6 +118,7 @@ public: gms::feature secondary_indexes_on_static_columns { *this, "SECONDARY_INDEXES_ON_STATIC_COLUMNS"sv }; gms::feature tablets { *this, "TABLETS"sv }; gms::feature uuid_sstable_identifiers { *this, "UUID_SSTABLE_IDENTIFIERS"sv }; + gms::feature table_digest_insensitive_to_expiry { *this, "TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"sv }; // A feature just for use in tests. It must not be advertised unless // the "features_enable_test_feature" injection is enabled. diff --git a/schema_mutations.cc b/schema_mutations.cc index 02ce9a643dc7..92525d2be08d 100644 --- a/schema_mutations.cc +++ b/schema_mutations.cc @@ -49,7 +49,7 @@ void schema_mutations::copy_to(std::vector& dst) const { } } -table_schema_version schema_mutations::digest() const { +table_schema_version schema_mutations::digest(db::schema_features sf) const { if (_scylla_tables) { auto rs = query::result_set(*_scylla_tables); if (!rs.empty()) { @@ -62,16 +62,19 @@ table_schema_version schema_mutations::digest() const { } md5_hasher h; - db::schema_features sf = db::schema_features::full(); - - // Disable this feature so that the digest remains compactible with Scylla - // versions prior to this feature. - // This digest affects the table schema version calculation and it's important - // that all nodes arrive at the same table schema version to avoid needless schema version - // pulls. Table schema versions are calculated on boot when we don't yet - // know all the cluster features, so we could get different table versions after reboot - // in an already upgraded cluster. - sf.remove(); + + if (!sf.contains()) { + // Disable this feature so that the digest remains compactible with Scylla + // versions prior to this feature. + // This digest affects the table schema version calculation and it's important + // that all nodes arrive at the same table schema version to avoid needless schema version + // pulls. It used to be the case that when table schema versions were calculated on boot we + // didn't yet know all the cluster features, so we could get different table versions after reboot + // in an already upgraded cluster. However, they are now available, and if + // TABLE_DIGEST_INSENSITIVE_TO_EXPIRY is enabled, we can compute with DIGEST_INSENSITIVE_TO_EXPIRY + // enabled. + sf.remove(); + } db::schema_tables::feed_hash_for_schema_digest(h, _columnfamilies, sf); db::schema_tables::feed_hash_for_schema_digest(h, _columns, sf); diff --git a/schema_mutations.hh b/schema_mutations.hh index efc0c1a6dcef..183082b6a851 100644 --- a/schema_mutations.hh +++ b/schema_mutations.hh @@ -12,6 +12,7 @@ #include "mutation/mutation.hh" #include "schema/schema_fwd.hh" #include "mutation/canonical_mutation.hh" +#include "db/schema_features.hh" // Commutative representation of table schema // Equality ignores tombstones. @@ -124,7 +125,7 @@ public: bool is_view() const; - table_schema_version digest() const; + table_schema_version digest(db::schema_features) const; std::optional partitioner() const; bool operator==(const schema_mutations&) const; diff --git a/service/migration_manager.cc b/service/migration_manager.cc index 0e8ba3fda8b8..bc95109d8659 100644 --- a/service/migration_manager.cc +++ b/service/migration_manager.cc @@ -109,6 +109,14 @@ void migration_manager::init_messaging_service() _feature_listeners.push_back(_feat.cdc.when_enabled(update_schema)); _feature_listeners.push_back(_feat.per_table_partitioners.when_enabled(update_schema)); _feature_listeners.push_back(_feat.computed_columns.when_enabled(update_schema)); + + if (!_feat.table_digest_insensitive_to_expiry) { + _feature_listeners.push_back(_feat.table_digest_insensitive_to_expiry.when_enabled([this] { + (void) with_gate(_background_tasks, [this] { + return reload_schema(); + }); + })); + } } _messaging.register_definitions_update([this] (const rpc::client_info& cinfo, std::vector fm, rpc::optional> cm) { diff --git a/test/boost/schema_change_test.cc b/test/boost/schema_change_test.cc index cab8e7c85d06..9a64f61c57dd 100644 --- a/test/boost/schema_change_test.cc +++ b/test/boost/schema_change_test.cc @@ -812,8 +812,6 @@ future<> test_schema_digest_does_not_change_with_disabled_features(sstring data_ expect_digest(sf, expected_digests[4]); - // FIXME: schema_mutations::digest() is still sensitive to expiry, so we can check versions only after forward_jump_clocks() - // otherwise the results would not be stable. expect_version("tests", "table1", expected_digests[5]); expect_version("ks", "tbl", expected_digests[6]); expect_version("ks", "tbl_view", expected_digests[7]); @@ -840,7 +838,8 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change) { utils::UUID("de49e92f-a00d-3f24-8779-d07de26708cb"), }; return test_schema_digest_does_not_change_with_disabled_features("./test/resource/sstables/schema_digest_test", - std::set{"COMPUTED_COLUMNS", "CDC", "KEYSPACE_STORAGE_OPTIONS"}, std::move(expected_digests), [] (cql_test_env& e) {}); + std::set{"COMPUTED_COLUMNS", "CDC", "KEYSPACE_STORAGE_OPTIONS", "TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"}, + std::move(expected_digests), [] (cql_test_env& e) {}); } SEASTAR_TEST_CASE(test_schema_digest_does_not_change_after_computed_columns) { @@ -857,7 +856,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_after_computed_columns) { utils::UUID("94606636-ae43-3e0a-b238-e7f0e33ef600"), }; return test_schema_digest_does_not_change_with_disabled_features("./test/resource/sstables/schema_digest_test_computed_columns", - std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS"}, std::move(expected_digests), [] (cql_test_env& e) {}); + std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS", "TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"}, std::move(expected_digests), [] (cql_test_env& e) {}); } SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_functions) { @@ -875,7 +874,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_functions) { }; return test_schema_digest_does_not_change_with_disabled_features( "./test/resource/sstables/schema_digest_with_functions_test", - std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS"}, + std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS", "TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"}, std::move(expected_digests), [] (cql_test_env& e) { e.execute_cql("create function twice(val int) called on null input returns int language lua as 'return 2 * val';").get(); @@ -900,7 +899,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_cdc_options) { }; return test_schema_digest_does_not_change_with_disabled_features( "./test/resource/sstables/schema_digest_test_cdc_options", - std::set{"KEYSPACE_STORAGE_OPTIONS"}, + std::set{"KEYSPACE_STORAGE_OPTIONS", "TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"}, std::move(expected_digests), [] (cql_test_env& e) { e.execute_cql("create table tests.table_cdc (pk int primary key, c1 int, c2 int) with cdc = {'enabled':'true'};").get(); @@ -923,7 +922,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_keyspace_storage_optio }; return test_schema_digest_does_not_change_with_disabled_features( "./test/resource/sstables/schema_digest_test_keyspace_storage_options", - std::set{}, + std::set{"TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"}, std::move(expected_digests), [] (cql_test_env& e) { e.execute_cql("create keyspace tests_s3 with replication = { 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1 }" diff --git a/test/boost/schema_registry_test.cc b/test/boost/schema_registry_test.cc index 09c61eed0d78..862430258b13 100644 --- a/test/boost/schema_registry_test.cc +++ b/test/boost/schema_registry_test.cc @@ -34,10 +34,12 @@ static schema_ptr random_schema() { struct dummy_init { std::unique_ptr config; + gms::feature_service fs; - dummy_init() { - config = std::make_unique(); - local_schema_registry().init(db::schema_ctxt(*config,std::make_shared())); + dummy_init() + : config(std::make_unique()) + , fs(gms::feature_config_from_db_config(*config)) { + local_schema_registry().init(db::schema_ctxt(*config, std::make_shared(), fs)); } }; diff --git a/tools/schema_loader.cc b/tools/schema_loader.cc index c4312a89aa47..3ca25f53c6be 100644 --- a/tools/schema_loader.cc +++ b/tools/schema_loader.cc @@ -534,7 +534,9 @@ schema_ptr do_load_schema_from_schema_tables(std::filesystem::path scylla_data_p db::config dbcfg; auto user_type_storage = std::make_shared(std::move(utm)); - db::schema_ctxt ctxt(dbcfg, user_type_storage); + gms::feature_service features(gms::feature_config_from_db_config(dbcfg)); + db::schema_ctxt ctxt(dbcfg, user_type_storage, features); + schema_mutations muts(std::move(*tables), std::move(*columns), std::move(view_virtual_columns), std::move(computed_columns), std::move(indexes), std::move(dropped_columns), std::move(scylla_tables)); return db::schema_tables::create_table_from_mutations(ctxt, muts); From 1ecd3c1a9a7e037a5b8917e13231407fe2e380d6 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Mon, 3 Jul 2023 20:32:03 +0200 Subject: [PATCH 4/4] test: schema_change_test: Verify digests also with TABLE_DIGEST_INSENSITIVE_TO_EXPIRY enabled The new test cases are a mirror of old test cases, but with updated digests. --- test/boost/schema_change_test.cc | 116 +++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 5 deletions(-) diff --git a/test/boost/schema_change_test.cc b/test/boost/schema_change_test.cc index 9a64f61c57dd..6e59870f2bb3 100644 --- a/test/boost/schema_change_test.cc +++ b/test/boost/schema_change_test.cc @@ -824,7 +824,7 @@ future<> test_schema_digest_does_not_change_with_disabled_features(sstring data_ }, cfg_in).then([tmp = std::move(tmp)] {}); } -SEASTAR_TEST_CASE(test_schema_digest_does_not_change) { +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_without_digest_feature) { std::vector expected_digests{ utils::UUID("264f79fc-61bd-3670-8d6e-2794f9787b0a"), utils::UUID("d2035515-b299-3265-b920-7dbe5306e72a"), @@ -842,7 +842,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change) { std::move(expected_digests), [] (cql_test_env& e) {}); } -SEASTAR_TEST_CASE(test_schema_digest_does_not_change_after_computed_columns) { +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_after_computed_columns_without_digest_feature) { std::vector expected_digests{ utils::UUID("036153ec-4565-34fb-a878-ce347b94f247"), utils::UUID("fa2e7735-7604-3202-8ce9-399996305aca"), @@ -859,7 +859,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_after_computed_columns) { std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS", "TABLE_DIGEST_INSENSITIVE_TO_EXPIRY"}, std::move(expected_digests), [] (cql_test_env& e) {}); } -SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_functions) { +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_functions_without_digest_feature) { std::vector expected_digests{ utils::UUID("6fa38d16-bbc4-3da5-bda5-680329789d8f"), utils::UUID("649bf7ec-fd64-3ccb-adde-3887fc1432be"), @@ -882,7 +882,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_functions) { }); } -SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_cdc_options) { +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_cdc_options_without_digest_feature) { auto ext = std::make_shared(); ext->add_schema_extension(cdc::cdc_extension::NAME); std::vector expected_digests{ @@ -907,7 +907,7 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_cdc_options) { std::move(ext)); } -SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_keyspace_storage_options) { +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_keyspace_storage_options_without_digest_feature) { std::vector expected_digests{ utils::UUID("d9f78213-ff9f-3208-9083-47e18cebf06f"), utils::UUID("30e2cf99-389d-381f-82b9-3fcdcf66a1fb"), @@ -930,6 +930,112 @@ SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_keyspace_storage_optio e.execute_cql("create table tests_s3.table1 (pk int primary key, c1 int, c2 int)").get(); }); } +SEASTAR_TEST_CASE(test_schema_digest_does_not_change) { + std::vector expected_digests{ + utils::UUID("264f79fc-61bd-3670-8d6e-2794f9787b0a"), + utils::UUID("d2035515-b299-3265-b920-7dbe5306e72a"), + utils::UUID("d2035515-b299-3265-b920-7dbe5306e72a"), + utils::UUID("de49e92f-a00d-3f24-8779-d07de26708cb"), + utils::UUID("de49e92f-a00d-3f24-8779-d07de26708cb"), + utils::UUID("75550bef-3a95-3901-be80-4540f7d8a311"), + utils::UUID("aff80a2a-4a72-35bb-9ac3-f851013610d0"), + utils::UUID("030100b2-27aa-32f2-8964-0090a1af75f8"), + utils::UUID("16ba4b2d-7c61-393b-ba51-0890e25f4e22"), + utils::UUID("de49e92f-a00d-3f24-8779-d07de26708cb"), + }; + return test_schema_digest_does_not_change_with_disabled_features("./test/resource/sstables/schema_digest_test", + std::set{"COMPUTED_COLUMNS", "CDC", "KEYSPACE_STORAGE_OPTIONS"}, + std::move(expected_digests), [] (cql_test_env& e) {}); +} + +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_after_computed_columns) { + std::vector expected_digests{ + utils::UUID("036153ec-4565-34fb-a878-ce347b94f247"), + utils::UUID("fa2e7735-7604-3202-8ce9-399996305aca"), + utils::UUID("fa2e7735-7604-3202-8ce9-399996305aca"), + utils::UUID("94606636-ae43-3e0a-b238-e7f0e33ef600"), + utils::UUID("94606636-ae43-3e0a-b238-e7f0e33ef600"), + utils::UUID("5a89ff92-9b5c-32eb-ad5a-5def856e3024"), + utils::UUID("26808d79-e22a-3d20-88a7-d812301ff342"), + utils::UUID("371527f3-2f26-32a6-8b29-bb0ce0735b61"), + utils::UUID("02ed06b1-c384-3f83-b116-fe94f5bf647a"), + utils::UUID("94606636-ae43-3e0a-b238-e7f0e33ef600"), + }; + return test_schema_digest_does_not_change_with_disabled_features("./test/resource/sstables/schema_digest_test_computed_columns", + std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS"}, std::move(expected_digests), [] (cql_test_env& e) {}); +} + +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_functions) { + std::vector expected_digests{ + utils::UUID("6fa38d16-bbc4-3da5-bda5-680329789d8f"), + utils::UUID("649bf7ec-fd64-3ccb-adde-3887fc1432be"), + utils::UUID("649bf7ec-fd64-3ccb-adde-3887fc1432be"), + utils::UUID("48fd0c1b-9777-34be-8c16-187c6ab55cfc"), + utils::UUID("48fd0c1b-9777-34be-8c16-187c6ab55cfc"), + utils::UUID("9b842fb8-2b89-3f9f-a344-f648cb27a226"), + utils::UUID("e596cbc4-60f8-3788-96e6-fdfb105ba39f"), + utils::UUID("0f214b9c-81a5-3771-8722-4763ab8fd0ee"), + utils::UUID("08624ebc-c0d2-3e7a-bcd7-4fcb442626e4"), + utils::UUID("48fd0c1b-9777-34be-8c16-187c6ab55cfc"), + }; + return test_schema_digest_does_not_change_with_disabled_features( + "./test/resource/sstables/schema_digest_with_functions_test", + std::set{"CDC", "KEYSPACE_STORAGE_OPTIONS"}, + std::move(expected_digests), + [] (cql_test_env& e) { + e.execute_cql("create function twice(val int) called on null input returns int language lua as 'return 2 * val';").get(); + e.execute_cql("create function my_add(a int, b int) called on null input returns int language lua as 'return a + b';").get(); + }); +} + +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_cdc_options) { + auto ext = std::make_shared(); + ext->add_schema_extension(cdc::cdc_extension::NAME); + std::vector expected_digests{ + utils::UUID("ff69e387-64ca-3335-b488-b7a615908148"), + utils::UUID("7f1ac621-fc68-3420-bc9b-54520da40418"), + utils::UUID("7f1ac621-fc68-3420-bc9b-54520da40418"), + utils::UUID("09899769-4e7f-3119-9769-e3db3d99455b"), + utils::UUID("09899769-4e7f-3119-9769-e3db3d99455b"), + utils::UUID("fdfdea09-fee9-3fd4-945f-b91a7a2e0e39"), + utils::UUID("44e79540-5cc5-3617-88c0-267fe7cc2232"), + utils::UUID("e2b673e7-04c0-37cb-b076-77951f2f5452"), + utils::UUID("089d5e42-065a-3e19-a608-58a960816c51"), + utils::UUID("09899769-4e7f-3119-9769-e3db3d99455b"), + }; + return test_schema_digest_does_not_change_with_disabled_features( + "./test/resource/sstables/schema_digest_test_cdc_options", + std::set{"KEYSPACE_STORAGE_OPTIONS"}, + std::move(expected_digests), + [] (cql_test_env& e) { + e.execute_cql("create table tests.table_cdc (pk int primary key, c1 int, c2 int) with cdc = {'enabled':'true'};").get(); + }, + std::move(ext)); +} + +SEASTAR_TEST_CASE(test_schema_digest_does_not_change_with_keyspace_storage_options) { + std::vector expected_digests{ + utils::UUID("d9f78213-ff9f-3208-9083-47e18cebf06f"), + utils::UUID("30e2cf99-389d-381f-82b9-3fcdcf66a1fb"), + utils::UUID("30e2cf99-389d-381f-82b9-3fcdcf66a1fb"), + utils::UUID("98d63879-6633-3708-880e-8716fcbadda0"), + utils::UUID("98d63879-6633-3708-880e-8716fcbadda0"), + utils::UUID("1f971ee2-42d1-3564-ae89-0090803d6d58"), + utils::UUID("60444aca-708a-387f-b571-e4c0806ab78d"), + utils::UUID("11c00de3-d47f-38bd-84f1-0f5e1179a168"), + utils::UUID("c495feac-b2a4-3c50-91a5-363630f878d6"), + utils::UUID("3fc03c97-8010-3746-8cea-e8b9ac27fe4e"), + }; + return test_schema_digest_does_not_change_with_disabled_features( + "./test/resource/sstables/schema_digest_test_keyspace_storage_options", + std::set{}, + std::move(expected_digests), + [] (cql_test_env& e) { + e.execute_cql("create keyspace tests_s3 with replication = { 'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1 }" + " and storage = { 'type': 'S3', 'bucket': 'b1', 'endpoint': 'localhost' };").get(); + e.execute_cql("create table tests_s3.table1 (pk int primary key, c1 int, c2 int)").get(); + }); +} // Regression test, ensuring people don't forget to set the null sharder // for newly added schema tables.