From b9f49d31dff96f8883f7691095fda628bd77fec3 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 30 Jul 2020 22:08:13 +0300 Subject: [PATCH 1/4] Sanity checks for MergeTreeSettings --- programs/server/Server.cpp | 3 +++ src/Storages/MergeTree/MergeTreeData.cpp | 4 +++ src/Storages/MergeTree/MergeTreeSettings.cpp | 27 +++++++++++++++++++ src/Storages/MergeTree/MergeTreeSettings.h | 4 +++ ...merge_tree_settings_sanity_check.reference | 0 ...01419_merge_tree_settings_sanity_check.sql | 21 +++++++++++++++ 6 files changed, 59 insertions(+) create mode 100644 tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.reference create mode 100644 tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index c3b17824151..c0bb1746cdb 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -641,6 +641,9 @@ int Server::main(const std::vector & /*args*/) global_context->setFormatSchemaPath(format_schema_path.path()); format_schema_path.createDirectories(); + /// Check sanity of MergeTreeSettings on server startup + global_context->getMergeTreeSettings().sanityCheck(settings); + /// Limit on total memory usage size_t max_server_memory_usage = config().getUInt64("max_server_memory_usage", 0); diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 740d44605ee..7f5773024ca 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -147,6 +147,10 @@ MergeTreeData::MergeTreeData( if (relative_data_path.empty()) throw Exception("MergeTree storages require data path", ErrorCodes::INCORRECT_FILE_NAME); + /// Check sanity of MergeTreeSettings. Only when table is created. + if (!attach) + settings->sanityCheck(global_context.getSettingsRef()); + MergeTreeDataFormatVersion min_format_version(0); if (!date_column_name.empty()) { diff --git a/src/Storages/MergeTree/MergeTreeSettings.cpp b/src/Storages/MergeTree/MergeTreeSettings.cpp index 5c4113c1565..ff5d15bc3f1 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.cpp +++ b/src/Storages/MergeTree/MergeTreeSettings.cpp @@ -76,4 +76,31 @@ void MergeTreeSettings::loadFromQuery(ASTStorage & storage_def) #undef ADD_IF_ABSENT } +void MergeTreeSettings::sanityCheck(const Settings & query_settings) const +{ + if (number_of_free_entries_in_pool_to_execute_mutation >= query_settings.background_pool_size) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, "The value of 'number_of_free_entries_in_pool_to_execute_mutation' setting" + " ({}) (default values are defined in section of config.xml" + " or the value can be specified per table in SETTINGS section of CREATE TABLE query)" + " is greater or equals to the value of 'background_pool_size'" + " ({}) (the value is defined in users.xml for default profile)." + " This indicates incorrect configuration because mutations cannot work with these settings.", + number_of_free_entries_in_pool_to_execute_mutation, + query_settings.background_pool_size); + } + + if (number_of_free_entries_in_pool_to_lower_max_size_of_merge >= query_settings.background_pool_size) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, "The value of 'number_of_free_entries_in_pool_to_lower_max_size_of_merge' setting" + " ({}) (default values are defined in section of config.xml" + " or the value can be specified per table in SETTINGS section of CREATE TABLE query)" + " is greater or equals to the value of 'background_pool_size'" + " ({}) (the value is defined in users.xml for default profile)." + " This indicates incorrect configuration because the maximum size of merge will be always lowered.", + number_of_free_entries_in_pool_to_execute_mutation, + query_settings.background_pool_size); + } +} + } diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index 833425ff592..e8c817895d1 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -18,6 +18,7 @@ namespace DB { class ASTStorage; +struct Settings; /** Settings for the MergeTree family of engines. * Could be loaded from config or from a CREATE TABLE query (SETTINGS clause). @@ -127,6 +128,9 @@ struct MergeTreeSettings : public SettingsCollection return name == "min_bytes_for_wide_part" || name == "min_rows_for_wide_part" || name == "min_bytes_for_compact_part" || name == "min_rows_for_compact_part"; } + + /// Check that the values are sane taking also query-level settings into account. + void sanityCheck(const Settings & query_settings) const; }; using MergeTreeSettingsPtr = std::shared_ptr; diff --git a/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.reference b/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql b/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql new file mode 100644 index 00000000000..525fff9b256 --- /dev/null +++ b/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql @@ -0,0 +1,21 @@ +CREATE TABLE mytable_local +( + created DateTime, + eventday Date, + user_id UInt32 +) +ENGINE = MergeTree() +PARTITION BY toYYYYMM(eventday) +ORDER BY (eventday, user_id) +SETTINGS number_of_free_entries_in_pool_to_execute_mutation = 100; -- { serverError 36 } + +CREATE TABLE mytable_local +( + created DateTime, + eventday Date, + user_id UInt32 +) +ENGINE = MergeTree() +PARTITION BY toYYYYMM(eventday) +ORDER BY (eventday, user_id) +SETTINGS number_of_free_entries_in_pool_to_lower_max_size_of_merge = 100; -- { serverError 36 } From 2c40539df64453daac914f914f74a3d688e5e3d7 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Fri, 31 Jul 2020 00:42:55 +0300 Subject: [PATCH 2/4] Check ALTERs; update test --- src/Storages/MergeTree/MergeTreeData.cpp | 6 +++++- .../01415_inconsistent_merge_tree_settings.sql | 2 +- .../01419_merge_tree_settings_sanity_check.sql | 16 ++++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 7f5773024ca..d1d4dae59a7 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -1471,7 +1471,6 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const S if (old_metadata.hasSettingsChanges()) { - const auto current_changes = old_metadata.getSettingsChanges()->as().changes; const auto & new_changes = new_metadata.settings_changes->as().changes; for (const auto & changed_setting : new_changes) @@ -1619,6 +1618,7 @@ void MergeTreeData::changeSettings( const auto & new_changes = new_settings->as().changes; for (const auto & change : new_changes) + { if (change.name == "storage_policy") { StoragePolicyPtr new_storage_policy = global_context.getStoragePolicy(change.value.safeGet()); @@ -1653,9 +1653,13 @@ void MergeTreeData::changeSettings( has_storage_policy_changed = true; } } + } MergeTreeSettings copy = *getSettings(); copy.applyChanges(new_changes); + + copy.sanityCheck(global_context.getSettingsRef()); + storage_settings.set(std::make_unique(copy)); StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); new_metadata.setSettingsChanges(new_settings); diff --git a/tests/queries/0_stateless/01415_inconsistent_merge_tree_settings.sql b/tests/queries/0_stateless/01415_inconsistent_merge_tree_settings.sql index f3bf24193a8..2ce0575c4ad 100644 --- a/tests/queries/0_stateless/01415_inconsistent_merge_tree_settings.sql +++ b/tests/queries/0_stateless/01415_inconsistent_merge_tree_settings.sql @@ -1,7 +1,7 @@ DROP TABLE IF EXISTS t; SET mutations_sync = 1; -CREATE TABLE t (x UInt8, s String) ENGINE = MergeTree ORDER BY x SETTINGS number_of_free_entries_in_pool_to_execute_mutation = 1000; +CREATE TABLE t (x UInt8, s String) ENGINE = MergeTree ORDER BY x SETTINGS number_of_free_entries_in_pool_to_execute_mutation = 15; INSERT INTO t VALUES (1, 'hello'); SELECT * FROM t; diff --git a/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql b/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql index 525fff9b256..686594f435d 100644 --- a/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql +++ b/tests/queries/0_stateless/01419_merge_tree_settings_sanity_check.sql @@ -1,3 +1,5 @@ +DROP TABLE IF EXISTS mytable_local; + CREATE TABLE mytable_local ( created DateTime, @@ -19,3 +21,17 @@ ENGINE = MergeTree() PARTITION BY toYYYYMM(eventday) ORDER BY (eventday, user_id) SETTINGS number_of_free_entries_in_pool_to_lower_max_size_of_merge = 100; -- { serverError 36 } + +CREATE TABLE mytable_local +( + created DateTime, + eventday Date, + user_id UInt32 +) +ENGINE = MergeTree() +PARTITION BY toYYYYMM(eventday) +ORDER BY (eventday, user_id); + +ALTER TABLE mytable_local MODIFY SETTING number_of_free_entries_in_pool_to_execute_mutation = 100; -- { serverError 36 } + +DROP TABLE mytable_local; From b232659439c0e4fe07fc8f3d1eea49f15692cb17 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Mon, 3 Aug 2020 00:39:51 +0300 Subject: [PATCH 3/4] Update tests --- .../configs/forbid_background_merges.xml | 3 ++- .../configs/forbid_background_merges.xml | 3 ++- .../test_polymorphic_parts/configs/do_not_merge.xml | 1 - 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml b/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml index ffdca0cf6bc..f1f3af4224e 100644 --- a/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml +++ b/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml @@ -1,7 +1,8 @@ - 0 + 1 + 2 diff --git a/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml b/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml index ffdca0cf6bc..f1f3af4224e 100644 --- a/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml +++ b/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml @@ -1,7 +1,8 @@ - 0 + 1 + 2 diff --git a/tests/integration/test_polymorphic_parts/configs/do_not_merge.xml b/tests/integration/test_polymorphic_parts/configs/do_not_merge.xml index 8b57af4f48e..82aaeb1fbc8 100644 --- a/tests/integration/test_polymorphic_parts/configs/do_not_merge.xml +++ b/tests/integration/test_polymorphic_parts/configs/do_not_merge.xml @@ -2,7 +2,6 @@ 1 2 - 100 0 From eb2236fe8bbd49e7cf67106a6db6526b2505acd1 Mon Sep 17 00:00:00 2001 From: alesapin Date: Mon, 3 Aug 2020 14:13:43 +0300 Subject: [PATCH 4/4] Fix configs --- .../configs/forbid_background_merges.xml | 10 ++++------ tests/integration/test_insert_into_distributed/test.py | 2 +- .../configs/forbid_background_merges.xml | 10 ++++------ .../test.py | 2 +- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml b/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml index f1f3af4224e..bc2dae31ad6 100644 --- a/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml +++ b/tests/integration/test_insert_into_distributed/configs/forbid_background_merges.xml @@ -1,8 +1,6 @@ - - - 1 - 2 - - + + 1 + 2 + diff --git a/tests/integration/test_insert_into_distributed/test.py b/tests/integration/test_insert_into_distributed/test.py index e48584bac84..731ffbbe2fd 100644 --- a/tests/integration/test_insert_into_distributed/test.py +++ b/tests/integration/test_insert_into_distributed/test.py @@ -12,7 +12,7 @@ instance_test_reconnect = cluster.add_instance('instance_test_reconnect', main_c instance_test_inserts_batching = cluster.add_instance( 'instance_test_inserts_batching', main_configs=['configs/remote_servers.xml'], user_configs=['configs/enable_distributed_inserts_batching.xml']) -remote = cluster.add_instance('remote', user_configs=['configs/forbid_background_merges.xml']) +remote = cluster.add_instance('remote', main_configs=['configs/forbid_background_merges.xml']) instance_test_inserts_local_cluster = cluster.add_instance( 'instance_test_inserts_local_cluster', diff --git a/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml b/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml index f1f3af4224e..bc2dae31ad6 100644 --- a/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml +++ b/tests/integration/test_insert_into_distributed_through_materialized_view/configs/forbid_background_merges.xml @@ -1,8 +1,6 @@ - - - 1 - 2 - - + + 1 + 2 + diff --git a/tests/integration/test_insert_into_distributed_through_materialized_view/test.py b/tests/integration/test_insert_into_distributed_through_materialized_view/test.py index 2dc8d7326dd..1df803920f1 100644 --- a/tests/integration/test_insert_into_distributed_through_materialized_view/test.py +++ b/tests/integration/test_insert_into_distributed_through_materialized_view/test.py @@ -12,7 +12,7 @@ instance_test_reconnect = cluster.add_instance('instance_test_reconnect', main_c instance_test_inserts_batching = cluster.add_instance( 'instance_test_inserts_batching', main_configs=['configs/remote_servers.xml'], user_configs=['configs/enable_distributed_inserts_batching.xml']) -remote = cluster.add_instance('remote', user_configs=['configs/forbid_background_merges.xml']) +remote = cluster.add_instance('remote', main_configs=['configs/forbid_background_merges.xml']) instance_test_inserts_local_cluster = cluster.add_instance( 'instance_test_inserts_local_cluster',