diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc index 2fad6c493c..8c766c4d81 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 a5c1e510a1..c9a7a5bbe1 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); } @@ -1489,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; }); @@ -3666,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) { @@ -3687,17 +3678,16 @@ 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. 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 38ee54d4eb..ef242fa0b1 100644 --- a/db/view/view.cc +++ b/db/view/view.cc @@ -575,15 +575,13 @@ 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) + 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) , _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)))}; } @@ -653,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)); @@ -1458,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) { } 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/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 bad089fafd..0931805c1d 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] { @@ -1106,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};