From 62e7a89f262909aa6d2c56ef9e88960d7d401500 Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 29 Jun 2022 17:53:08 +0200 Subject: [PATCH 1/4] Proper fix for ipv4/ipv6 conversion error --- src/Databases/TablesLoader.cpp | 13 +++++--- src/Databases/TablesLoader.h | 2 +- src/Interpreters/InterpreterCreateQuery.cpp | 5 +++ .../__init__.py | 1 + .../test.py | 31 +++++++++++++++++++ ...2316_cast_to_ip_address_default_column.sql | 17 +++++++++- 6 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 tests/integration/test_server_start_and_ip_conversions/__init__.py create mode 100644 tests/integration/test_server_start_and_ip_conversions/test.py diff --git a/src/Databases/TablesLoader.cpp b/src/Databases/TablesLoader.cpp index e973c9211be..7e9b83d423a 100644 --- a/src/Databases/TablesLoader.cpp +++ b/src/Databases/TablesLoader.cpp @@ -171,6 +171,11 @@ void TablesLoader::removeUnresolvableDependencies(bool remove_loaded) void TablesLoader::loadTablesInTopologicalOrder(ThreadPool & pool) { + /// Compatibility setting which should be enabled by default on attach + /// Otherwise server will be unable to start for some old-format of IPv6/IPv4 types of columns + ContextMutablePtr load_context = Context::createCopy(global_context); + load_context->setSetting("cast_ipv4_ipv6_default_on_conversion_error", 1); + /// Load independent tables in parallel. /// Then remove loaded tables from dependency graph, find tables/dictionaries that do not have unresolved dependencies anymore, /// move them to the list of independent tables and load. @@ -183,7 +188,7 @@ void TablesLoader::loadTablesInTopologicalOrder(ThreadPool & pool) assert(metadata.parsed_tables.size() == tables_processed + metadata.independent_database_objects.size() + getNumberOfTablesWithDependencies()); logDependencyGraph(); - startLoadingIndependentTables(pool, level); + startLoadingIndependentTables(pool, level, load_context); TableNames new_independent_database_objects; for (const auto & table_name : metadata.independent_database_objects) @@ -237,7 +242,7 @@ DependenciesInfosIter TablesLoader::removeResolvedDependency(const DependenciesI return metadata.dependencies_info.erase(info_it); } -void TablesLoader::startLoadingIndependentTables(ThreadPool & pool, size_t level) +void TablesLoader::startLoadingIndependentTables(ThreadPool & pool, size_t level, ContextMutablePtr load_context) { size_t total_tables = metadata.parsed_tables.size(); @@ -245,10 +250,10 @@ void TablesLoader::startLoadingIndependentTables(ThreadPool & pool, size_t level for (const auto & table_name : metadata.independent_database_objects) { - pool.scheduleOrThrowOnError([this, total_tables, &table_name]() + pool.scheduleOrThrowOnError([this, load_context, total_tables, &table_name]() { const auto & path_and_query = metadata.parsed_tables[table_name]; - databases[table_name.database]->loadTableFromMetadata(global_context, path_and_query.path, table_name, path_and_query.ast, force_restore); + databases[table_name.database]->loadTableFromMetadata(load_context, path_and_query.path, table_name, path_and_query.ast, force_restore); logAboutProgress(log, ++tables_processed, total_tables, stopwatch); }); } diff --git a/src/Databases/TablesLoader.h b/src/Databases/TablesLoader.h index 189906df6ff..43e8bfdb92c 100644 --- a/src/Databases/TablesLoader.h +++ b/src/Databases/TablesLoader.h @@ -104,7 +104,7 @@ private: DependenciesInfosIter removeResolvedDependency(const DependenciesInfosIter & info_it, TableNames & independent_database_objects); - void startLoadingIndependentTables(ThreadPool & pool, size_t level); + void startLoadingIndependentTables(ThreadPool & pool, size_t level, ContextMutablePtr load_context); void checkCyclicDependencies() const; diff --git a/src/Interpreters/InterpreterCreateQuery.cpp b/src/Interpreters/InterpreterCreateQuery.cpp index 20d88c91709..0fb048332b7 100644 --- a/src/Interpreters/InterpreterCreateQuery.cpp +++ b/src/Interpreters/InterpreterCreateQuery.cpp @@ -996,6 +996,11 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) throw Exception("Temporary tables cannot be inside a database. You should not specify a database for a temporary table.", ErrorCodes::BAD_DATABASE_FOR_TEMPORARY_TABLE); + /// Compatibility setting which should be enabled by default on attach + /// Otherwise server will be unable to start for some old-format of IPv6/IPv4 types + if (create.attach) + getContext()->setSetting("cast_ipv4_ipv6_default_on_conversion_error", 1); + String current_database = getContext()->getCurrentDatabase(); auto database_name = create.database ? create.getDatabase() : current_database; diff --git a/tests/integration/test_server_start_and_ip_conversions/__init__.py b/tests/integration/test_server_start_and_ip_conversions/__init__.py new file mode 100644 index 00000000000..e5a0d9b4834 --- /dev/null +++ b/tests/integration/test_server_start_and_ip_conversions/__init__.py @@ -0,0 +1 @@ +#!/usr/bin/env python3 diff --git a/tests/integration/test_server_start_and_ip_conversions/test.py b/tests/integration/test_server_start_and_ip_conversions/test.py new file mode 100644 index 00000000000..f91617f60b8 --- /dev/null +++ b/tests/integration/test_server_start_and_ip_conversions/test.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python3 +import logging +import pytest +from helpers.cluster import ClickHouseCluster + +cluster = ClickHouseCluster(__file__) +node = cluster.add_instance("node", stay_alive=True) + +@pytest.fixture(scope="module", autouse=True) +def start_cluster(): + try: + cluster.start() + yield cluster + finally: + cluster.shutdown() + + +def test_restart_success(): + node.query(""" + CREATE TABLE ipv4_test + ( + id UInt64, + value String + ) ENGINE=MergeTree ORDER BY id""", + settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + + node.query("ALTER TABLE ipv4_test MODIFY COLUMN value IPv4 DEFAULT ''", settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + + node.restart_clickhouse() + + assert node.query("SELECT 1") == "1\n" diff --git a/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql b/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql index 200cec8fed9..128acd7d132 100644 --- a/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql +++ b/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql @@ -11,6 +11,13 @@ CREATE TABLE ipv4_test ALTER TABLE ipv4_test MODIFY COLUMN value IPv4 DEFAULT ''; +SET cast_ipv4_ipv6_default_on_conversion_error = 0; + +DETACH TABLE ipv4_test; +ATTACH TABLE ipv4_test; + +SET cast_ipv4_ipv6_default_on_conversion_error = 1; + DROP TABLE ipv4_test; DROP TABLE IF EXISTS ipv6_test; @@ -20,7 +27,15 @@ CREATE TABLE ipv6_test value String ) ENGINE=MergeTree ORDER BY id; -ALTER TABLE ipv6_test MODIFY COLUMN value IPv4 DEFAULT ''; +ALTER TABLE ipv6_test MODIFY COLUMN value IPv6 DEFAULT ''; + +SET cast_ipv4_ipv6_default_on_conversion_error = 0; + +DETACH TABLE ipv6_test; +ATTACH TABLE ipv6_test; + +SET cast_ipv4_ipv6_default_on_conversion_error = 1; + SELECT * FROM ipv6_test; DROP TABLE ipv6_test; From 9b387a57ed79ff37a1530ac46592412799e8634b Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 29 Jun 2022 17:56:59 +0200 Subject: [PATCH 2/4] Test ipv6 as well --- .../test.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_server_start_and_ip_conversions/test.py b/tests/integration/test_server_start_and_ip_conversions/test.py index f91617f60b8..73604394d9d 100644 --- a/tests/integration/test_server_start_and_ip_conversions/test.py +++ b/tests/integration/test_server_start_and_ip_conversions/test.py @@ -15,7 +15,7 @@ def start_cluster(): cluster.shutdown() -def test_restart_success(): +def test_restart_success_ipv4(): node.query(""" CREATE TABLE ipv4_test ( @@ -29,3 +29,19 @@ def test_restart_success(): node.restart_clickhouse() assert node.query("SELECT 1") == "1\n" + + +def test_restart_success_ipv6(): + node.query(""" + CREATE TABLE ipv6_test + ( + id UInt64, + value String + ) ENGINE=MergeTree ORDER BY id""", + settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + + node.query("ALTER TABLE ipv6_test MODIFY COLUMN value IPv6 DEFAULT ''", settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + + node.restart_clickhouse() + + assert node.query("SELECT 1") == "1\n" From 8f5582f95efd18eac8c926a89574b20e617b939c Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 29 Jun 2022 20:29:50 +0200 Subject: [PATCH 3/4] Review and style fixes --- src/Interpreters/InterpreterCreateQuery.cpp | 9 ++++---- .../test.py | 23 ++++++++++++++----- ...2316_cast_to_ip_address_default_column.sql | 1 + 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/Interpreters/InterpreterCreateQuery.cpp b/src/Interpreters/InterpreterCreateQuery.cpp index 0fb048332b7..7a00bbf524c 100644 --- a/src/Interpreters/InterpreterCreateQuery.cpp +++ b/src/Interpreters/InterpreterCreateQuery.cpp @@ -996,11 +996,6 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) throw Exception("Temporary tables cannot be inside a database. You should not specify a database for a temporary table.", ErrorCodes::BAD_DATABASE_FOR_TEMPORARY_TABLE); - /// Compatibility setting which should be enabled by default on attach - /// Otherwise server will be unable to start for some old-format of IPv6/IPv4 types - if (create.attach) - getContext()->setSetting("cast_ipv4_ipv6_default_on_conversion_error", 1); - String current_database = getContext()->getCurrentDatabase(); auto database_name = create.database ? create.getDatabase() : current_database; @@ -1043,6 +1038,10 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) create.attach = true; create.attach_short_syntax = true; create.if_not_exists = if_not_exists; + + /// Compatibility setting which should be enabled by default on attach + /// Otherwise server will be unable to start for some old-format of IPv6/IPv4 types + getContext()->setSetting("cast_ipv4_ipv6_default_on_conversion_error", 1); } /// TODO throw exception if !create.attach_short_syntax && !create.attach_from_path && !internal diff --git a/tests/integration/test_server_start_and_ip_conversions/test.py b/tests/integration/test_server_start_and_ip_conversions/test.py index 73604394d9d..abb6a546f64 100644 --- a/tests/integration/test_server_start_and_ip_conversions/test.py +++ b/tests/integration/test_server_start_and_ip_conversions/test.py @@ -6,6 +6,7 @@ from helpers.cluster import ClickHouseCluster cluster = ClickHouseCluster(__file__) node = cluster.add_instance("node", stay_alive=True) + @pytest.fixture(scope="module", autouse=True) def start_cluster(): try: @@ -16,15 +17,20 @@ def start_cluster(): def test_restart_success_ipv4(): - node.query(""" + node.query( + """ CREATE TABLE ipv4_test ( id UInt64, value String ) ENGINE=MergeTree ORDER BY id""", - settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}, + ) - node.query("ALTER TABLE ipv4_test MODIFY COLUMN value IPv4 DEFAULT ''", settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + node.query( + "ALTER TABLE ipv4_test MODIFY COLUMN value IPv4 DEFAULT ''", + settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}, + ) node.restart_clickhouse() @@ -32,15 +38,20 @@ def test_restart_success_ipv4(): def test_restart_success_ipv6(): - node.query(""" + node.query( + """ CREATE TABLE ipv6_test ( id UInt64, value String ) ENGINE=MergeTree ORDER BY id""", - settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}, + ) - node.query("ALTER TABLE ipv6_test MODIFY COLUMN value IPv6 DEFAULT ''", settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}) + node.query( + "ALTER TABLE ipv6_test MODIFY COLUMN value IPv6 DEFAULT ''", + settings={"cast_ipv4_ipv6_default_on_conversion_error": 1}, + ) node.restart_clickhouse() diff --git a/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql b/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql index 128acd7d132..eabe6ed1d65 100644 --- a/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql +++ b/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql @@ -1,4 +1,5 @@ -- Tags: no-backward-compatibility-check +-- TODO: remove after new 22.6 release SET cast_ipv4_ipv6_default_on_conversion_error = 1; From 615070425e8c759e282af1c3917c4cab6482b89b Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 29 Jun 2022 20:30:23 +0200 Subject: [PATCH 4/4] Fix comment --- .../0_stateless/02316_cast_to_ip_address_default_column.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql b/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql index eabe6ed1d65..35f210be43d 100644 --- a/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql +++ b/tests/queries/0_stateless/02316_cast_to_ip_address_default_column.sql @@ -1,5 +1,5 @@ -- Tags: no-backward-compatibility-check --- TODO: remove after new 22.6 release +-- TODO: remove no-backward-compatibility-check after new 22.6 release SET cast_ipv4_ipv6_default_on_conversion_error = 1;