From 99272087e69b760b7df9ec965a1dd33cf3b4537c Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 19:08:16 +0200 Subject: [PATCH 1/7] schema_mutations: Use mutation_opt instead of stdx::optional --- db/schema_tables.cc | 2 +- schema_mutations.cc | 26 +++++++++++++------------- schema_mutations.hh | 22 +++++++++++----------- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 719a4d58b9..ac6f1b29f2 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -1943,7 +1943,7 @@ schema_ptr create_table_from_mutations(schema_mutations sm, std::experimental::o std::vector index_defs; if (sm.indices_mutation()) { - index_defs = create_indices_from_index_rows(query::result_set(sm.indices_mutation().value()), ks_name, cf_name); + index_defs = create_indices_from_index_rows(query::result_set(*sm.indices_mutation()), ks_name, cf_name); } for (auto&& index : index_defs) { builder.with_index(index); diff --git a/schema_mutations.cc b/schema_mutations.cc index cda846691c..898652cc7a 100644 --- a/schema_mutations.cc +++ b/schema_mutations.cc @@ -32,22 +32,22 @@ schema_mutations::schema_mutations(canonical_mutation columnfamilies, stdx::optional scylla_tables) : _columnfamilies(columnfamilies.to_mutation(is_view ? db::schema_tables::views() : db::schema_tables::tables())) , _columns(columns.to_mutation(db::schema_tables::columns())) - , _indices(indices ? stdx::optional{indices.value().to_mutation(db::schema_tables::indexes())} : stdx::nullopt) - , _dropped_columns(dropped_columns ? stdx::optional{dropped_columns.value().to_mutation(db::schema_tables::dropped_columns())} : stdx::nullopt) - , _scylla_tables(scylla_tables ? stdx::optional{scylla_tables.value().to_mutation(db::schema_tables::scylla_tables())} : stdx::nullopt) + , _indices(indices ? mutation_opt{indices.value().to_mutation(db::schema_tables::indexes())} : stdx::nullopt) + , _dropped_columns(dropped_columns ? mutation_opt{dropped_columns.value().to_mutation(db::schema_tables::dropped_columns())} : stdx::nullopt) + , _scylla_tables(scylla_tables ? mutation_opt{scylla_tables.value().to_mutation(db::schema_tables::scylla_tables())} : stdx::nullopt) {} void schema_mutations::copy_to(std::vector& dst) const { dst.push_back(_columnfamilies); dst.push_back(_columns); if (_indices) { - dst.push_back(_indices.value()); + dst.push_back(*_indices); } if (_dropped_columns) { - dst.push_back(_dropped_columns.value()); + dst.push_back(*_dropped_columns); } if (_scylla_tables) { - dst.push_back(_scylla_tables.value()); + dst.push_back(*_scylla_tables); } } @@ -68,26 +68,26 @@ table_schema_version schema_mutations::digest() const { md5_hasher h; db::schema_tables::feed_hash_for_schema_digest(h, _columnfamilies); db::schema_tables::feed_hash_for_schema_digest(h, _columns); - if (_indices && !_indices.value().partition().empty()) { - db::schema_tables::feed_hash_for_schema_digest(h, _indices.value()); + if (_indices && !_indices->partition().empty()) { + db::schema_tables::feed_hash_for_schema_digest(h, *_indices); } - if (_dropped_columns && !_dropped_columns.value().partition().empty()) { - db::schema_tables::feed_hash_for_schema_digest(h, _dropped_columns.value()); + if (_dropped_columns && !_dropped_columns->partition().empty()) { + db::schema_tables::feed_hash_for_schema_digest(h, *_dropped_columns); } if (_scylla_tables) { - db::schema_tables::feed_hash_for_schema_digest(h, _scylla_tables.value()); + db::schema_tables::feed_hash_for_schema_digest(h, *_scylla_tables); } return utils::UUID_gen::get_name_UUID(h.finalize()); } -static stdx::optional compact(const stdx::optional& m) { +static mutation_opt compact(const mutation_opt& m) { if (!m) { return m; } return db::schema_tables::compact_for_schema_digest(*m); } -static stdx::optional compact(const mutation& m) { +static mutation_opt compact(const mutation& m) { return db::schema_tables::compact_for_schema_digest(m); } diff --git a/schema_mutations.hh b/schema_mutations.hh index 90e8fd7346..70f4027413 100644 --- a/schema_mutations.hh +++ b/schema_mutations.hh @@ -31,12 +31,12 @@ class schema_mutations { mutation _columnfamilies; mutation _columns; - stdx::optional _indices; - stdx::optional _dropped_columns; - stdx::optional _scylla_tables; + mutation_opt _indices; + mutation_opt _dropped_columns; + mutation_opt _scylla_tables; public: - schema_mutations(mutation columnfamilies, mutation columns, stdx::optional indices, stdx::optional dropped_columns, - stdx::optional scylla_tables) + schema_mutations(mutation columnfamilies, mutation columns, mutation_opt indices, mutation_opt dropped_columns, + mutation_opt scylla_tables) : _columnfamilies(std::move(columnfamilies)) , _columns(std::move(columns)) , _indices(std::move(indices)) @@ -65,14 +65,14 @@ public: return _columns; } - const stdx::optional& scylla_tables() const { + const mutation_opt& scylla_tables() const { return _scylla_tables; } - const stdx::optional& indices_mutation() const { + const mutation_opt& indices_mutation() const { return _indices; } - const stdx::optional& dropped_columns_mutation() const { + const mutation_opt& dropped_columns_mutation() const { return _dropped_columns; } @@ -86,19 +86,19 @@ public: stdx::optional indices_canonical_mutation() const { if (_indices) { - return canonical_mutation(_indices.value()); + return canonical_mutation(*_indices); } return {}; } stdx::optional dropped_columns_canonical_mutation() const { if (_dropped_columns) { - return canonical_mutation(_dropped_columns.value()); + return canonical_mutation(*_dropped_columns); } return {}; } stdx::optional scylla_tables_canonical_mutation() const { if (_scylla_tables) { - return canonical_mutation(_scylla_tables.value()); + return canonical_mutation(*_scylla_tables); } return {}; } From f943d2efbf763381062fd0604115fc2e9b98e59b Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 19:09:18 +0200 Subject: [PATCH 2/7] schema_tables: Don't alter tables which differ only in version We apply deletion of scylla_tables.version to the incoming schema mutations so that table schema version is recalculated after merge. The mutations which we read from local schema tables may not have it deleted in which case all tables would be considered as differing on the presence of the version field. Avoid this by deleting the field from old mutations as well. --- db/schema_tables.cc | 9 +++++++++ schema_mutations.hh | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index ac6f1b29f2..1a5a246f21 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -801,6 +801,15 @@ static future<> do_merge_schema(distributed& proxy, std: /*auto& old_aggregates = */read_schema_for_keyspaces(proxy, AGGREGATES, keyspaces).get0(); #endif + // Incoming mutations have the version field deleted. Delete here as well so that + // schemas which are otherwise equal don't appear as differing. + for (auto&& e : old_column_families) { + schema_mutations& sm = e.second; + if (sm.scylla_tables()) { + delete_schema_version(*sm.scylla_tables()); + } + } + proxy.local().mutate_locally(std::move(mutations)).get0(); if (do_flush) { diff --git a/schema_mutations.hh b/schema_mutations.hh index 70f4027413..a9d38fdabe 100644 --- a/schema_mutations.hh +++ b/schema_mutations.hh @@ -69,6 +69,10 @@ public: return _scylla_tables; } + mutation_opt& scylla_tables() { + return _scylla_tables; + } + const mutation_opt& indices_mutation() const { return _indices; } From 713d75fd518157fb8f021278e0b4154f9be2e491 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 19:15:41 +0200 Subject: [PATCH 3/7] storage_service: Introduce SCHEMA_TABLES_V3 feature --- service/storage_service.cc | 3 +++ service/storage_service.hh | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/service/storage_service.cc b/service/storage_service.cc index 531f84a185..3e3f0e7c2d 100644 --- a/service/storage_service.cc +++ b/service/storage_service.cc @@ -89,6 +89,7 @@ static const sstring COUNTERS_FEATURE = "COUNTERS"; static const sstring INDEXES_FEATURE = "INDEXES"; static const sstring DIGEST_MULTIPARTITION_READ_FEATURE = "DIGEST_MULTIPARTITION_READ"; static const sstring CORRECT_COUNTER_ORDER_FEATURE = "CORRECT_COUNTER_ORDER"; +static const sstring SCHEMA_TABLES_V3 = "SCHEMA_TABLES_V3"; distributed _the_storage_service; @@ -131,6 +132,7 @@ sstring storage_service::get_config_supported_features() { COUNTERS_FEATURE, DIGEST_MULTIPARTITION_READ_FEATURE, CORRECT_COUNTER_ORDER_FEATURE, + SCHEMA_TABLES_V3 }; if (service::get_local_storage_service()._db.local().get_config().experimental()) { features.push_back(MATERIALIZED_VIEWS_FEATURE); @@ -1357,6 +1359,7 @@ future<> storage_service::init_server(int delay) { ss._counters_feature = gms::feature(COUNTERS_FEATURE); ss._digest_multipartition_read_feature = gms::feature(DIGEST_MULTIPARTITION_READ_FEATURE); ss._correct_counter_order_feature = gms::feature(CORRECT_COUNTER_ORDER_FEATURE); + ss._schema_tables_v3 = gms::feature(SCHEMA_TABLES_V3); if (ss._db.local().get_config().experimental()) { ss._materialized_views_feature = gms::feature(MATERIALIZED_VIEWS_FEATURE); diff --git a/service/storage_service.hh b/service/storage_service.hh index dd198e1942..8f055ea8f6 100644 --- a/service/storage_service.hh +++ b/service/storage_service.hh @@ -265,6 +265,7 @@ private: gms::feature _indexes_feature; gms::feature _digest_multipartition_read_feature; gms::feature _correct_counter_order_feature; + gms::feature _schema_tables_v3; public: void enable_all_features() { _range_tombstones_feature.enable(); @@ -274,6 +275,7 @@ public: _indexes_feature.enable(); _digest_multipartition_read_feature.enable(); _correct_counter_order_feature.enable(); + _schema_tables_v3.enable(); } void finish_bootstrapping() { @@ -2247,6 +2249,10 @@ public: bool cluster_supports_correct_counter_order() const { return bool(_correct_counter_order_feature); } + + bool cluster_supports_schema_tables_v3() const { + return bool(_schema_tables_v3); + } }; inline future<> init_storage_service(distributed& db) { From 5a92c18e634e30c75179c82858c8a4d3ae3b4503 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 19:16:30 +0200 Subject: [PATCH 4/7] migration_manager: Disable pulls during rolling upgrade from 1.7 If there is a schema pull during rolling upgrade among a two 2.0 nodes, then schema merge will delete the persisted schema version. When the node loads that table again, e.g. on restart, it will generate a version which is different than the one which 1.7 nodes use. This will cause reads and writes to fail. To avoid this, disable pulls until all nodes are upgraded. Fixes #2802. --- service/migration_manager.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/service/migration_manager.cc b/service/migration_manager.cc index 41f3c01476..bcaaaac695 100644 --- a/service/migration_manager.cc +++ b/service/migration_manager.cc @@ -261,7 +261,10 @@ bool migration_manager::has_compatible_schema_tables_version(const gms::inet_add bool migration_manager::should_pull_schema_from(const gms::inet_address& endpoint) { return has_compatible_schema_tables_version(endpoint) - && !gms::get_local_gossiper().is_gossip_only_member(endpoint); + && !gms::get_local_gossiper().is_gossip_only_member(endpoint) + // Disable pulls during rolling upgrade from 1.7 to 2.0 to avoid + // schema version inconsistency. See https://github.com/scylladb/scylla/issues/2802. + && get_storage_service().local().cluster_supports_schema_tables_v3(); } future<> migration_manager::notify_create_keyspace(const lw_shared_ptr& ksm) { From 571cac95ed9f6b32004706ae0665830a645d0a8a Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 20:16:39 +0200 Subject: [PATCH 5/7] schema_tables: Make make_scylla_tables_mutation() visible For tests. --- db/schema_tables.cc | 2 +- db/schema_tables.hh | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 1a5a246f21..802f7fc8fe 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -1522,7 +1522,7 @@ static void add_dropped_column_to_schema_mutation(schema_ptr table, const sstrin m.set_clustered_cell(ckey, "type", expand_user_type(column.type)->as_cql3_type()->to_string(), timestamp); } -static mutation make_scylla_tables_mutation(schema_ptr table, api::timestamp_type timestamp) { +mutation make_scylla_tables_mutation(schema_ptr table, api::timestamp_type timestamp) { schema_ptr s = tables(); auto pkey = partition_key::from_singular(*s, table->ks_name()); auto ckey = clustering_key::from_singular(*s, table->cf_name()); diff --git a/db/schema_tables.hh b/db/schema_tables.hh index 63597c28a2..08146ec799 100644 --- a/db/schema_tables.hh +++ b/db/schema_tables.hh @@ -165,6 +165,7 @@ view_ptr create_view_from_mutations(schema_mutations, std::experimental::optiona future> create_views_from_schema_partition(distributed& proxy, const schema_result::mapped_type& result); schema_mutations make_schema_mutations(schema_ptr s, api::timestamp_type timestamp, bool with_columns); +mutation make_scylla_tables_mutation(schema_ptr, api::timestamp_type timestamp); void add_table_or_view_to_schema_mutation(schema_ptr view, api::timestamp_type timestamp, bool with_columns, std::vector& mutations); From f0fdf75e7ce45b4d249919ca857515181062e440 Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 20:17:54 +0200 Subject: [PATCH 6/7] tests: cql_test_env: Enable all features in tests --- tests/cql_test_env.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/cql_test_env.cc b/tests/cql_test_env.cc index 8c75a72b9e..6451f36ab1 100644 --- a/tests/cql_test_env.cc +++ b/tests/cql_test_env.cc @@ -309,6 +309,10 @@ public: gms::get_failure_detector().stop().get(); }); + ss.invoke_on_all([] (auto&& ss) { + ss.enable_all_features(); + }).get(); + distributed& proxy = service::get_storage_proxy(); distributed& mm = service::get_migration_manager(); distributed& bm = db::get_batchlog_manager(); From c657eec4cf5c3203c756a8d219652cb89da3074c Mon Sep 17 00:00:00 2001 From: Tomasz Grabiec Date: Thu, 14 Sep 2017 20:17:01 +0200 Subject: [PATCH 7/7] tests: schema_change_test: Add test_merging_does_not_alter_tables_which_didnt_change test case --- tests/schema_change_test.cc | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/schema_change_test.cc b/tests/schema_change_test.cc index e4f4525a2f..cd1902b141 100644 --- a/tests/schema_change_test.cc +++ b/tests/schema_change_test.cc @@ -31,6 +31,7 @@ #include "tests/result_set_assertions.hh" #include "service/migration_manager.hh" #include "schema_builder.hh" +#include "schema_registry.hh" #include "disk-error-handler.hh" @@ -246,6 +247,49 @@ SEASTAR_TEST_CASE(test_combined_column_add_and_drop) { }); } +SEASTAR_TEST_CASE(test_merging_does_not_alter_tables_which_didnt_change) { + return do_with_cql_env([](cql_test_env& e) { + return seastar::async([&] { + service::migration_manager& mm = service::get_local_migration_manager(); + + auto&& keyspace = e.db().local().find_keyspace("ks").metadata(); + + auto legacy_version = utils::UUID_gen::get_time_UUID(); + auto s0 = schema_builder("ks", "table1") + .with_column("pk", bytes_type, column_kind::partition_key) + .with_column("v1", bytes_type) + .with_version(legacy_version) + .build(); + + auto find_table = [&] () -> column_family& { + return e.db().local().find_column_family("ks", "table1"); + }; + + auto muts1 = db::schema_tables::make_create_table_mutations(keyspace, s0, api::new_timestamp()).get0(); + service::get_storage_proxy().local().mutate_locally(muts1).get(); + e.db().invoke_on_all([gs = global_schema_ptr(s0)] (database& db) { + return db.add_column_family_and_make_directory(gs); + }).get(); + + auto s1 = find_table().schema(); + + BOOST_REQUIRE_EQUAL(legacy_version, s1->version()); + + mm.announce(muts1).get(); + + BOOST_REQUIRE(s1 == find_table().schema()); + BOOST_REQUIRE_EQUAL(legacy_version, find_table().schema()->version()); + + auto muts2 = muts1; + muts2.push_back(db::schema_tables::make_scylla_tables_mutation(s0, api::new_timestamp())); + mm.announce(muts2).get(); + + BOOST_REQUIRE(s1 == find_table().schema()); + BOOST_REQUIRE_EQUAL(legacy_version, find_table().schema()->version()); + }); + }); +} + class counting_migration_listener : public service::migration_listener { public: int create_keyspace_count = 0;