From 5397524875d8f3d6a7914e5c22b348d8bc253bb9 Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Wed, 11 Oct 2023 12:52:36 +0200 Subject: [PATCH 1/4] feature_service: make COMPUTED_COLUMNS feature unconditionally true The feature is assumed to be true, it was introduced in 2019. It's still advertised in gossip, but it's assumed to always be present. The `schema_feature` enum class still contains `COMPUTED_COLUMNS`, and the `all_tables` function in schema_tables.cc still checks for the schema feature when deciding if `computed_columns()` table should be included. This is necessary because digest calculation tests contain many digests calculated with the feature disabled, if we wanted to make it unconditional in the schema_tables code we'd have to regenerate almost all digests in the tests. It is simpler to leave the possibility for the tests to disable the feature. --- db/schema_tables.cc | 7 ------- gms/feature_service.cc | 3 ++- gms/feature_service.hh | 1 - service/migration_manager.cc | 1 - 4 files changed, 2 insertions(+), 10 deletions(-) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index a5c1e510a1..f26ab7a844 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -829,13 +829,6 @@ future> convert_schema_to_mutations(distributed< std::vector adjust_schema_for_schema_features(std::vector schema, schema_features features) { - //Don't send the `computed_columns` table mutations to nodes that doesn't know it. - if (!features.contains(schema_feature::COMPUTED_COLUMNS)) { - schema.erase(std::remove_if(schema.begin(), schema.end(), [] (const mutation& m) { - return m.schema()->cf_name() == COMPUTED_COLUMNS; - }) , schema.end()); - } - for (auto& m : schema) { m = redact_columns_for_missing_features(m, features); } diff --git a/gms/feature_service.cc b/gms/feature_service.cc index 75a2f9dfa9..a823fb0103 100644 --- a/gms/feature_service.cc +++ b/gms/feature_service.cc @@ -138,6 +138,7 @@ std::set feature_service::supported_feature_set() const { "CORRECT_STATIC_COMPACT_IN_MC"sv, "UNBOUNDED_RANGE_TOMBSTONES"sv, "MC_SSTABLE_FORMAT"sv, + "COMPUTED_COLUMNS"sv, }; if (is_test_only_feature_deprecated()) { @@ -195,7 +196,7 @@ db::schema_features feature_service::cluster_schema_features() const { db::schema_features f; f.set_if(view_virtual_columns); f.set_if(digest_insensitive_to_expiry); - f.set_if(computed_columns); + f.set(); f.set_if(cdc); f.set_if(per_table_partitioners); f.set_if(keyspace_storage_options); diff --git a/gms/feature_service.hh b/gms/feature_service.hh index f753da36a0..6f11427386 100644 --- a/gms/feature_service.hh +++ b/gms/feature_service.hh @@ -87,7 +87,6 @@ public: gms::feature me_sstable { *this, "ME_SSTABLE_FORMAT"sv }; gms::feature view_virtual_columns { *this, "VIEW_VIRTUAL_COLUMNS"sv }; gms::feature digest_insensitive_to_expiry { *this, "DIGEST_INSENSITIVE_TO_EXPIRY"sv }; - gms::feature computed_columns { *this, "COMPUTED_COLUMNS"sv }; gms::feature cdc { *this, "CDC"sv }; gms::feature nonfrozen_udts { *this, "NONFROZEN_UDTS"sv }; gms::feature hinted_handoff_separate_connection { *this, "HINTED_HANDOFF_SEPARATE_CONNECTION"sv }; diff --git a/service/migration_manager.cc b/service/migration_manager.cc index bad089fafd..d421cb0894 100644 --- a/service/migration_manager.cc +++ b/service/migration_manager.cc @@ -109,7 +109,6 @@ void migration_manager::init_messaging_service() _feature_listeners.push_back(_feat.digest_insensitive_to_expiry.when_enabled(update_schema)); _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] { From f02ac9a9e786059dff3584826b6166fb19e2df7e Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Wed, 11 Oct 2023 13:54:39 +0200 Subject: [PATCH 2/4] schema_tables: make comment more precise `maybe_fix_legacy_secondary_index_mv_schema` function has this piece of code: ``` // If the first clustering key part of a view is a column with name not found in base schema, // it implies it might be backing an index created before computed columns were introduced, // and as such it must be recreated properly. if (!base_schema->columns_by_name().contains(first_view_ck.name())) { schema_builder builder{schema_ptr(v)}; builder.mark_column_computed(first_view_ck.name(), std::make_unique()); if (preserve_version) { builder.with_version(v->version()); } return view_ptr(builder.build()); } ``` The comment uses the phrase "it might be". However, the code inside the `if` assumes that it "must be": once we determined that the first column in this materialized view does not have a corresponding name in the base table, we set it to be computed using `legacy_token_column_computation`, so we assumed that the column was indeed storing the token. Doing that for a column which is not the token column would be a small disaster. Assuming that the code is correct, we can make the comment more precise. I checked the documentation and I don't see any other way how we could have such a column other than the token column which is internally created by Scylla when creating a secondary index (for example, it is forbidden to use an alias in select statement when creating materialized views, which I checked experimentally). --- db/schema_tables.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index f26ab7a844..4a556e9cfe 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -3680,8 +3680,8 @@ view_ptr maybe_fix_legacy_secondary_index_mv_schema(replica::database& db, const } // If the first clustering key part of a view is a column with name not found in base schema, - // it implies it might be backing an index created before computed columns were introduced, - // and as such it must be recreated properly. + // and the column is not computed (which we checked above), then it must be backing an index + // created before computed columns were introduced, and as such it must be recreated properly. if (!base_schema->columns_by_name().contains(first_view_ck.name())) { schema_builder builder{schema_ptr(v)}; builder.mark_column_computed(first_view_ck.name(), std::make_unique()); From 3976808b12a543fe274cf8aff1882635287a29d7 Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Wed, 11 Oct 2023 14:48:23 +0200 Subject: [PATCH 3/4] schema_tables: turn view schema fixing code into a sanity check The purpose of `maybe_fix_legacy_secondary_index_mv_schema` was to deal with legacy materialized view schemas used for secondary indexes, schemas which were created before the notion of "computed columns" was introduced. Back then, secondary index schemas would use a regular "token" column. Later it became a computed column and old schemas would be migrated during rolling upgrade. The migration code was introduced in 2019 (db8d4a0cc649c74e30198dd83b082b0ad50b1314) and then fixed in 2020 (d473bc9b0669e9d3879ff0f329a9ed16378dd569). The fix was present in Enterprise 2022.1 and in OSS 4.5. So, assuming that users don't try crazy things like upgrading from 2021.X to 2023.X (which we do not support), all clusters will have already executed the migration code once they upgrade to 2023.X, meaning we can get rid of it. The main motivation of this patch is to get rid of the `db::schema_tables::merge_schema` call in `parse_schema_tables`. In Raft mode this was the only call to `merge_schema` outside "group 0 code" and in fact it is unsafe -- it uses locally generated mutations with locally generated timestamp (`api::new_timestamp()`), so if we actually did it, we would permanently diverge the group 0 state machine across nodes (the schema pulling code is disabled in Raft mode). Fortunately, this should be dead code by now, as explained in the previous paragraph. The migration code is now turned into a sanity check, if the users try something crazy, they will get an error instead of silent data corruption. --- cql3/statements/select_statement.cc | 12 +++------ db/schema_tables.cc | 41 +++++++++++++---------------- db/schema_tables.hh | 4 +-- db/view/view.cc | 24 +++++++---------- replica/database.cc | 13 ++------- service/migration_manager.cc | 8 ++---- 6 files changed, 37 insertions(+), 65 deletions(-) diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc index f947b4eecc..f1f9d10b1b 100644 --- a/cql3/statements/select_statement.cc +++ b/cql3/statements/select_statement.cc @@ -1003,16 +1003,12 @@ static void append_base_key_to_index_ck(std::vector& explode bytes indexed_table_select_statement::compute_idx_token(const partition_key& key) const { const column_definition& cdef = *_view_schema->clustering_key_columns().begin(); - clustering_row empty_row(clustering_key_prefix::make_empty()); - bytes computed_value; if (!cdef.is_computed()) { - // FIXME(pgrabowski): this legacy code is here for backward compatibility and should be removed - // once "computed_columns feature" is supported by every node - computed_value = legacy_token_column_computation().compute_value(*_schema, key); - } else { - computed_value = cdef.get_computation().compute_value(*_schema, key); + throw std::logic_error{format( + "Detected legacy non-computed token column {} in table {}.{}", + cdef.name_as_text(), _schema->ks_name(), _schema->cf_name())}; } - return computed_value; + return cdef.get_computation().compute_value(*_schema, key); } lw_shared_ptr indexed_table_select_statement::generate_view_paging_state_from_base_query_results(lw_shared_ptr paging_state, diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 4a556e9cfe..c9a7a5bbe1 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -1482,12 +1482,10 @@ static future<> merge_tables_and_views(distributed& prox base_schema = proxy.local().local_db().find_schema(vp->ks_name(), vp->view_info()->base_name()); } - // Now when we have a referenced base - just in case we are registering an old view (this can happen in a mixed cluster) - // lets make it write enabled by updating it's compute columns. - view_ptr fixed_vp = maybe_fix_legacy_secondary_index_mv_schema(proxy.local().get_db().local(), vp, base_schema, preserve_version::yes); - if(fixed_vp) { - vp = fixed_vp; - } + // Now when we have a referenced base - sanity check that we're not registering an old view + // (this could happen when we skip multiple major versions in upgrade, which is unsupported.) + check_no_legacy_secondary_index_mv_schema(proxy.local().get_db().local(), vp, base_schema); + vp->view_info()->set_base_info(vp->view_info()->make_base_dependent_view_info(*base_schema)); return vp; }); @@ -3659,20 +3657,20 @@ std::vector all_table_names(schema_features features) { boost::adaptors::transformed([] (auto schema) { return schema->cf_name(); })); } -view_ptr maybe_fix_legacy_secondary_index_mv_schema(replica::database& db, const view_ptr& v, schema_ptr base_schema, preserve_version preserve_version) { +void check_no_legacy_secondary_index_mv_schema(replica::database& db, const view_ptr& v, schema_ptr base_schema) { // Legacy format for a secondary index used a hardcoded "token" column, which ensured a proper - // order for indexed queries. This "token" column is now implemented as a computed column, - // but for the sake of compatibility we assume that there might be indexes created in the legacy - // format, where "token" is not marked as computed. Once we're sure that all indexes have their - // columns marked as computed (because they were either created on a node that supports computed - // columns or were fixed by this utility function), it's safe to remove this function altogether. + // order for indexed queries. This "token" column is has been implemented as a computed column + // for a long time now, and migration code has been / will be executed on all reasonable Scylla + // deployments (which don't do unsupported upgrades). + // + // This function is now used as a sanity check that we're not dealing with the legacy format anymore. if (v->clustering_key_size() == 0) { - return view_ptr(nullptr); + return; } const auto ck_cols = v->clustering_key_columns(); const column_definition& first_view_ck = ck_cols.front(); if (first_view_ck.is_computed()) { - return view_ptr(nullptr); + return; } if (!base_schema) { @@ -3681,16 +3679,15 @@ view_ptr maybe_fix_legacy_secondary_index_mv_schema(replica::database& db, const // If the first clustering key part of a view is a column with name not found in base schema, // and the column is not computed (which we checked above), then it must be backing an index - // created before computed columns were introduced, and as such it must be recreated properly. + // created before computed columns were introduced. if (!base_schema->columns_by_name().contains(first_view_ck.name())) { - schema_builder builder{schema_ptr(v)}; - builder.mark_column_computed(first_view_ck.name(), std::make_unique()); - if (preserve_version) { - builder.with_version(v->version()); - } - return view_ptr(builder.build()); + on_fatal_internal_error(slogger, format( + "Materialized view {}.{}: first clustering key column ({}) is not computed and does not have a corresponding" + " column in the base table. This materialized view must therefore be a secondary index created" + " using legacy method (without computed columns) that wasn't migrated properly to new method." + " Make sure that you perform rolling upgrade according to documented procedure without skipping" + " major Scylla versions.", v->ks_name(), v->cf_name(), first_view_ck.name_as_text())); } - return view_ptr(nullptr); } diff --git a/db/schema_tables.hh b/db/schema_tables.hh index 196d7b0e24..f9e1811bfa 100644 --- a/db/schema_tables.hh +++ b/db/schema_tables.hh @@ -264,9 +264,7 @@ std::vector make_update_view_mutations(lw_shared_ptr make_drop_view_mutations(lw_shared_ptr keyspace, view_ptr view, api::timestamp_type timestamp); -class preserve_version_tag {}; -using preserve_version = bool_class; -view_ptr maybe_fix_legacy_secondary_index_mv_schema(replica::database& db, const view_ptr& v, schema_ptr base_schema, preserve_version preserve_version); +void check_no_legacy_secondary_index_mv_schema(replica::database& db, const view_ptr& v, schema_ptr base_schema); sstring serialize_kind(column_kind kind); column_kind deserialize_kind(sstring kind); diff --git a/db/view/view.cc b/db/view/view.cc index f4245e3786..3450b2e65d 100644 --- a/db/view/view.cc +++ b/db/view/view.cc @@ -575,7 +575,6 @@ private: const partition_key& _base_key; const clustering_or_static_row& _update; const std::optional& _existing; - const bool _backing_secondary_index; public: value_getter(const schema& base, const partition_key& base_key, const clustering_or_static_row& update, const std::optional& existing, bool backing_secondary_index) @@ -583,7 +582,6 @@ public: , _base_key(base_key) , _update(update) , _existing(existing) - , _backing_secondary_index(backing_secondary_index) { } @@ -616,22 +614,18 @@ public: private: vector_type handle_computed_column(const column_definition& cdef) { - bytes computed_value; if (!cdef.is_computed()) { - //FIXME(sarna): this legacy code is here for backward compatibility and should be removed - // once "computed_columns feature" is supported by every node - if (!_backing_secondary_index) { - throw std::logic_error(format("Column {} doesn't exist in base and this view is not backing a secondary index", cdef.name_as_text())); - } - computed_value = legacy_token_column_computation().compute_value(_base, _base_key); - } else { - auto& computation = cdef.get_computation(); - if (auto* collection_computation = dynamic_cast(&computation)) { - return handle_collection_column_computation(collection_computation); - } - computed_value = computation.compute_value(_base, _base_key); + throw std::logic_error{format( + "Detected legacy non-computed token column {} in view for table {}.{}", + cdef.name_as_text(), _base.ks_name(), _base.cf_name())}; } + auto& computation = cdef.get_computation(); + if (auto* collection_computation = dynamic_cast(&computation)) { + return handle_collection_column_computation(collection_computation); + } + + auto computed_value = computation.compute_value(_base, _base_key); return {managed_bytes_view(linearized_values.emplace_back(std::move(computed_value)))}; } diff --git a/replica/database.cc b/replica/database.cc index ea4c43a956..2f362bf1e3 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -852,17 +852,8 @@ future<> database::parse_system_tables(distributed& prox co_await do_parse_schema_tables(proxy, db::schema_tables::VIEWS, coroutine::lambda([&] (schema_result_value_type &v) -> future<> { std::vector views = co_await create_views_from_schema_partition(proxy, v.second); co_await coroutine::parallel_for_each(views.begin(), views.end(), [&] (auto&& v) -> future<> { - // TODO: Remove once computed columns are guaranteed to be featured in the whole cluster. - // we fix here the schema in place in oreder to avoid races (write commands comming from other coordinators). - view_ptr fixed_v = maybe_fix_legacy_secondary_index_mv_schema(*this, v, nullptr, preserve_version::yes); - view_ptr v_to_add = fixed_v ? fixed_v : v; - co_await this->add_column_family_and_make_directory(v_to_add, replica::database::is_new_cf::no); - if (bool(fixed_v)) { - v_to_add = fixed_v; - auto&& keyspace = find_keyspace(v->ks_name()).metadata(); - auto mutations = db::schema_tables::make_update_view_mutations(keyspace, view_ptr(v), fixed_v, api::new_timestamp(), true); - co_await db::schema_tables::merge_schema(sys_ks, proxy, _feat, std::move(mutations)); - } + check_no_legacy_secondary_index_mv_schema(*this, v, nullptr); + co_await this->add_column_family_and_make_directory(v, replica::database::is_new_cf::no); }); })); } diff --git a/service/migration_manager.cc b/service/migration_manager.cc index d421cb0894..0931805c1d 100644 --- a/service/migration_manager.cc +++ b/service/migration_manager.cc @@ -1105,15 +1105,11 @@ static future get_schema_definition(table_schema_version v, netw::me // That means the column mapping for the schema should always be inserted // with TTL (refresh TTL in case column mapping already existed prior to that). auto us = s.unfreeze(db::schema_ctxt(proxy)); - // if this is a view - we might need to fix it's schema before registering it. + // if this is a view - sanity check that its schema doesn't need fixing. if (us->is_view()) { auto& db = proxy.local().local_db(); schema_ptr base_schema = db.find_schema(us->view_info()->base_id()); - auto fixed_view = db::schema_tables::maybe_fix_legacy_secondary_index_mv_schema(db, view_ptr(us), base_schema, - db::schema_tables::preserve_version::yes); - if (fixed_view) { - us = fixed_view; - } + db::schema_tables::check_no_legacy_secondary_index_mv_schema(db, view_ptr(us), base_schema); } return db::schema_tables::store_column_mapping(proxy, us, true).then([us] { return frozen_schema{us}; From db49ccccb05c2e6cdcf25a18469ead75b85e32cf Mon Sep 17 00:00:00 2001 From: Kamil Braun Date: Wed, 11 Oct 2023 15:09:11 +0200 Subject: [PATCH 4/4] view: remove unused `_backing_secondary_index` This boolean was only used for a sanity check which was replaced with a stronger sanity check in the previous commit that doesn't require the boolean. --- db/view/view.cc | 7 +++---- db/view/view.hh | 4 +--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/db/view/view.cc b/db/view/view.cc index 3450b2e65d..96c49ae50e 100644 --- a/db/view/view.cc +++ b/db/view/view.cc @@ -577,7 +577,7 @@ private: const std::optional& _existing; public: - value_getter(const schema& base, const partition_key& base_key, const clustering_or_static_row& update, const std::optional& existing, bool backing_secondary_index) + value_getter(const schema& base, const partition_key& base_key, const clustering_or_static_row& update, const std::optional& existing) : _base(base) , _base_key(base_key) , _update(update) @@ -647,7 +647,7 @@ private: std::vector view_updates::get_view_rows(const partition_key& base_key, const clustering_or_static_row& update, const std::optional& existing) { - value_getter getter(*_base, base_key, update, existing, _backing_secondary_index); + value_getter getter(*_base, base_key, update, existing); auto get_value = boost::adaptors::transformed(std::ref(getter)); @@ -1452,8 +1452,7 @@ view_update_builder make_view_update_builder( " base schema version of the view ({}) for view {}.{} of {}.{}", base->version(), v.base->base_schema()->version(), v.view->ks_name(), v.view->cf_name(), base->ks_name(), base->cf_name())); } - bool is_index = base_table.get_index_manager().is_index(v.view); - return view_updates(std::move(v), is_index); + return view_updates(std::move(v)); })); return view_update_builder(std::move(db), base_table, base, std::move(vs), std::move(updates), std::move(existings), now); } diff --git a/db/view/view.hh b/db/view/view.hh index b8d578551f..84341c62db 100644 --- a/db/view/view.hh +++ b/db/view/view.hh @@ -210,15 +210,13 @@ class view_updates final { base_info_ptr _base_info; std::unordered_map _updates; mutable size_t _op_count = 0; - const bool _backing_secondary_index; public: - explicit view_updates(view_and_base vab, bool backing_secondary_index) + explicit view_updates(view_and_base vab) : _view(std::move(vab.view)) , _view_info(*_view->view_info()) , _base(vab.base->base_schema()) , _base_info(vab.base) , _updates(8, partition_key::hashing(*_view), partition_key::equality(*_view)) - , _backing_secondary_index(backing_secondary_index) { }