From fc0376a170904358e6b356b78342af1bb4af4bb7 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 28 Feb 2020 21:55:21 +0300 Subject: [PATCH] Shard now clamps the settings got from the initiator to the shard's constaints instead of throwing an exception. --- dbms/programs/server/TCPHandler.cpp | 79 ++++++++++--------- dbms/src/Access/SettingsConstraints.cpp | 71 +++++++++++++++++ dbms/src/Access/SettingsConstraints.h | 5 ++ dbms/src/Interpreters/Context.cpp | 18 ++++- dbms/src/Interpreters/Context.h | 6 +- .../configs/remote_servers.xml | 6 +- .../configs/users_on_cluster.xml | 44 +++++------ .../configs/users_on_distributed.xml | 44 ++++++----- .../test.py | 57 ++++++++++--- 9 files changed, 231 insertions(+), 99 deletions(-) diff --git a/dbms/programs/server/TCPHandler.cpp b/dbms/programs/server/TCPHandler.cpp index f3a22954796..243cd3bf086 100644 --- a/dbms/programs/server/TCPHandler.cpp +++ b/dbms/programs/server/TCPHandler.cpp @@ -875,48 +875,55 @@ void TCPHandler::receiveQuery() query_context->setCurrentQueryId(state.query_id); /// Client info + ClientInfo & client_info = query_context->getClientInfo(); + if (client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) + client_info.read(*in, client_revision); + + /// For better support of old clients, that does not send ClientInfo. + if (client_info.query_kind == ClientInfo::QueryKind::NO_QUERY) { - ClientInfo & client_info = query_context->getClientInfo(); - if (client_revision >= DBMS_MIN_REVISION_WITH_CLIENT_INFO) - client_info.read(*in, client_revision); - - /// For better support of old clients, that does not send ClientInfo. - if (client_info.query_kind == ClientInfo::QueryKind::NO_QUERY) - { - client_info.query_kind = ClientInfo::QueryKind::INITIAL_QUERY; - client_info.client_name = client_name; - client_info.client_version_major = client_version_major; - client_info.client_version_minor = client_version_minor; - client_info.client_version_patch = client_version_patch; - client_info.client_revision = client_revision; - } - - /// Set fields, that are known apriori. - client_info.interface = ClientInfo::Interface::TCP; - - if (client_info.query_kind == ClientInfo::QueryKind::INITIAL_QUERY) - { - /// 'Current' fields was set at receiveHello. - client_info.initial_user = client_info.current_user; - client_info.initial_query_id = client_info.current_query_id; - client_info.initial_address = client_info.current_address; - } - else - { - query_context->setInitialRowPolicy(); - } + client_info.query_kind = ClientInfo::QueryKind::INITIAL_QUERY; + client_info.client_name = client_name; + client_info.client_version_major = client_version_major; + client_info.client_version_minor = client_version_minor; + client_info.client_version_patch = client_version_patch; + client_info.client_revision = client_revision; } - /// Per query settings. - Settings custom_settings{}; + /// Set fields, that are known apriori. + client_info.interface = ClientInfo::Interface::TCP; + + if (client_info.query_kind == ClientInfo::QueryKind::INITIAL_QUERY) + { + /// 'Current' fields was set at receiveHello. + client_info.initial_user = client_info.current_user; + client_info.initial_query_id = client_info.current_query_id; + client_info.initial_address = client_info.current_address; + } + else + { + query_context->setInitialRowPolicy(); + } + + /// Per query settings are also passed via TCP. + /// We need to check them before applying due to they can violate the settings constraints. auto settings_format = (client_revision >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS) ? SettingsBinaryFormat::STRINGS : SettingsBinaryFormat::OLD; - custom_settings.deserialize(*in, settings_format); - auto settings_changes = custom_settings.changes(); - query_context->checkSettingsConstraints(settings_changes); + Settings passed_settings; + passed_settings.deserialize(*in, settings_format); + auto settings_changes = passed_settings.changes(); + if (client_info.query_kind == ClientInfo::QueryKind::INITIAL_QUERY) + { + /// Throw an exception if the passed settings violate the constraints. + query_context->checkSettingsConstraints(settings_changes); + } + else + { + /// Quietly clamp to the constraints if it's not an initial query. + query_context->clampToSettingsConstraints(settings_changes); + } query_context->applySettingsChanges(settings_changes); - - Settings & settings = query_context->getSettingsRef(); + const Settings & settings = query_context->getSettingsRef(); /// Sync timeouts on client and server during current query to avoid dangling queries on server /// NOTE: We use settings.send_timeout for the receive timeout and vice versa (change arguments ordering in TimeoutSetter), diff --git a/dbms/src/Access/SettingsConstraints.cpp b/dbms/src/Access/SettingsConstraints.cpp index 538b062b1e0..21ca2c8ab26 100644 --- a/dbms/src/Access/SettingsConstraints.cpp +++ b/dbms/src/Access/SettingsConstraints.cpp @@ -199,6 +199,77 @@ void SettingsConstraints::check(const Settings & current_settings, const Setting } +void SettingsConstraints::clamp(const Settings & current_settings, SettingChange & change) const +{ + const String & name = change.name; + size_t setting_index = Settings::findIndex(name); + if (setting_index == Settings::npos) + return; + + Field new_value = Settings::valueToCorrespondingType(setting_index, change.value); + Field current_value = current_settings.get(setting_index); + + /// Setting isn't checked if value wasn't changed. + if (current_value == new_value) + return; + + if (!current_settings.allow_ddl && name == "allow_ddl") + { + change.value = current_value; + return; + } + + /** The `readonly` value is understood as follows: + * 0 - everything allowed. + * 1 - only read queries can be made; you can not change the settings. + * 2 - You can only do read queries and you can change the settings, except for the `readonly` setting. + */ + if (current_settings.readonly == 1) + { + change.value = current_value; + return; + } + + if (current_settings.readonly > 1 && name == "readonly") + { + change.value = current_value; + return; + } + + const Constraint * constraint = tryGetConstraint(setting_index); + if (constraint) + { + if (constraint->read_only) + { + change.value = current_value; + return; + } + + if (!constraint->min_value.isNull() && (new_value < constraint->min_value)) + { + if (!constraint->max_value.isNull() && (constraint->min_value > constraint->max_value)) + change.value = current_value; + else + change.value = constraint->min_value; + return; + } + + if (!constraint->max_value.isNull() && (new_value > constraint->max_value)) + { + change.value = constraint->max_value; + return; + } + } +} + + +void SettingsConstraints::clamp(const Settings & current_settings, SettingsChanges & changes) const +{ + for (auto & change : changes) + clamp(current_settings, change); +} + + SettingsConstraints::Constraint & SettingsConstraints::getConstraintRef(size_t index) { auto it = constraints_by_index.find(index); diff --git a/dbms/src/Access/SettingsConstraints.h b/dbms/src/Access/SettingsConstraints.h index 3b4d0c28800..074dc66d123 100644 --- a/dbms/src/Access/SettingsConstraints.h +++ b/dbms/src/Access/SettingsConstraints.h @@ -85,9 +85,14 @@ public: Infos getInfo() const; + /// Checks whether `change` violates these constraints and throws an exception if so. void check(const Settings & current_settings, const SettingChange & change) const; void check(const Settings & current_settings, const SettingsChanges & changes) const; + /// Checks whether `change` violates these and clamps the `change` if so. + void clamp(const Settings & current_settings, SettingChange & change) const; + void clamp(const Settings & current_settings, SettingsChanges & changes) const; + /** Set multiple settings from "profile" (in server configuration file (users.xml), profiles contain groups of multiple settings). * The profile can also be set using the `set` functions, like the profile setting. */ diff --git a/dbms/src/Interpreters/Context.cpp b/dbms/src/Interpreters/Context.cpp index 572a6b97897..e02c2d566e2 100644 --- a/dbms/src/Interpreters/Context.cpp +++ b/dbms/src/Interpreters/Context.cpp @@ -1216,20 +1216,32 @@ void Context::applySettingsChanges(const SettingsChanges & changes) } -void Context::checkSettingsConstraints(const SettingChange & change) +void Context::checkSettingsConstraints(const SettingChange & change) const { if (settings_constraints) settings_constraints->check(settings, change); } - -void Context::checkSettingsConstraints(const SettingsChanges & changes) +void Context::checkSettingsConstraints(const SettingsChanges & changes) const { if (settings_constraints) settings_constraints->check(settings, changes); } +void Context::clampToSettingsConstraints(SettingChange & change) const +{ + if (settings_constraints) + settings_constraints->clamp(settings, change); +} + +void Context::clampToSettingsConstraints(SettingsChanges & changes) const +{ + if (settings_constraints) + settings_constraints->clamp(settings, changes); +} + + String Context::getCurrentDatabase() const { return current_database; diff --git a/dbms/src/Interpreters/Context.h b/dbms/src/Interpreters/Context.h index 40909a192a3..4bf72cd2b98 100644 --- a/dbms/src/Interpreters/Context.h +++ b/dbms/src/Interpreters/Context.h @@ -365,8 +365,10 @@ public: void applySettingsChanges(const SettingsChanges & changes); /// Checks the constraints. - void checkSettingsConstraints(const SettingChange & change); - void checkSettingsConstraints(const SettingsChanges & changes); + void checkSettingsConstraints(const SettingChange & change) const; + void checkSettingsConstraints(const SettingsChanges & changes) const; + void clampToSettingsConstraints(SettingChange & change) const; + void clampToSettingsConstraints(SettingsChanges & changes) const; /// Returns the current constraints (can return null). std::shared_ptr getSettingsConstraints() const { return settings_constraints; } diff --git a/dbms/tests/integration/test_settings_constraints_distributed/configs/remote_servers.xml b/dbms/tests/integration/test_settings_constraints_distributed/configs/remote_servers.xml index 5d8df3ac56d..faa56e0b6dc 100644 --- a/dbms/tests/integration/test_settings_constraints_distributed/configs/remote_servers.xml +++ b/dbms/tests/integration/test_settings_constraints_distributed/configs/remote_servers.xml @@ -5,12 +5,14 @@ node1 9000 - distributed + normal + + node2 9000 - distributed + readonly diff --git a/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_cluster.xml b/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_cluster.xml index e1a1122d9d6..11ba40ac50d 100644 --- a/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_cluster.xml +++ b/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_cluster.xml @@ -1,41 +1,33 @@ - 10000000000 - 0 - random - - 1000000 - 0 - random - 2 + + 50000000 - 1 - 1000000 + 11111111 + 99999999 - + + + 1 + + - default - default - - - distributed_profile - default - - ::/0 - - - - - - - + + normal + + + + readonly + + + diff --git a/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_distributed.xml b/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_distributed.xml index 15db7b92b15..371a6de26a3 100644 --- a/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_distributed.xml +++ b/dbms/tests/integration/test_settings_constraints_distributed/configs/users_on_distributed.xml @@ -1,34 +1,36 @@ - 1000000 - 0 - random - - 2000000000 + + 80000000 0 random - + + + 2000000000 + + + 1 + + - default - default - - - remote_profile - default - - ::/0 - - - - - - - + + normal + + + + wasteful + + + + readonly + + + diff --git a/dbms/tests/integration/test_settings_constraints_distributed/test.py b/dbms/tests/integration/test_settings_constraints_distributed/test.py index 7b73af2d5fe..b9dfb7619cb 100644 --- a/dbms/tests/integration/test_settings_constraints_distributed/test.py +++ b/dbms/tests/integration/test_settings_constraints_distributed/test.py @@ -8,8 +8,8 @@ from helpers.test_tools import assert_eq_with_retry cluster = ClickHouseCluster(__file__) -node1 = cluster.add_instance('node1', main_configs=['configs/remote_servers.xml'], user_configs=['configs/users_on_cluster.xml']) -node2 = cluster.add_instance('node2', main_configs=['configs/remote_servers.xml'], user_configs=['configs/users_on_cluster.xml']) +node1 = cluster.add_instance('node1', user_configs=['configs/users_on_cluster.xml']) +node2 = cluster.add_instance('node2', user_configs=['configs/users_on_cluster.xml']) distributed = cluster.add_instance('distributed', main_configs=['configs/remote_servers.xml'], user_configs=['configs/users_on_distributed.xml']) @@ -24,19 +24,58 @@ def started_cluster(): node.query("INSERT INTO sometable VALUES (toDate('2020-01-20'), 1, 1)") distributed.query("CREATE TABLE proxy (date Date, id UInt32, value Int32) ENGINE = Distributed(test_cluster, default, sometable);") + distributed.query("CREATE TABLE sysproxy (name String, value String) ENGINE = Distributed(test_cluster, system, settings);") yield cluster finally: cluster.shutdown() -def test_settings_under_remote(started_cluster): - assert distributed.query("SELECT COUNT() FROM proxy") == '1\n' - with pytest.raises(QueryRuntimeException): - distributed.query("SELECT COUNT() FROM proxy", user='remote') +def test_shard_doesnt_throw_on_constraint_violation(started_cluster): + query = "SELECT COUNT() FROM proxy" + assert distributed.query(query) == '2\n' + assert distributed.query(query, user = 'normal') == '2\n' + assert distributed.query(query, user = 'wasteful') == '2\n' + assert distributed.query(query, user = 'readonly') == '2\n' + + assert distributed.query(query, settings={"max_memory_usage": 40000000, "readonly": 2}) == '2\n' + assert distributed.query(query, settings={"max_memory_usage": 3000000000, "readonly": 2}) == '2\n' - assert distributed.query("SELECT COUNT() FROM proxy", settings={"max_memory_usage": 1000000}, user='remote') == '1\n' + query = "SELECT COUNT() FROM remote('node{1,2}', 'default', 'sometable')" + assert distributed.query(query) == '2\n' + assert distributed.query(query, user = 'normal') == '2\n' + assert distributed.query(query, user = 'wasteful') == '2\n' - with pytest.raises(QueryRuntimeException): - distributed.query("SELECT COUNT() FROM proxy", settings={"max_memory_usage": 1000001}, user='remote') + +def test_shard_clamps_settings(started_cluster): + query = "SELECT hostName() as host, name, value FROM sysproxy WHERE name = 'max_memory_usage' OR name = 'readonly' ORDER BY host, name, value" + assert distributed.query(query) == 'node1\tmax_memory_usage\t99999999\n'\ + 'node1\treadonly\t0\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n' + assert distributed.query(query, user = 'normal') == 'node1\tmax_memory_usage\t80000000\n'\ + 'node1\treadonly\t0\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n' + assert distributed.query(query, user = 'wasteful') == 'node1\tmax_memory_usage\t99999999\n'\ + 'node1\treadonly\t0\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n' + assert distributed.query(query, user = 'readonly') == 'node1\tmax_memory_usage\t99999999\n'\ + 'node1\treadonly\t1\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n' + + assert distributed.query(query, settings={"max_memory_usage": 1}) == 'node1\tmax_memory_usage\t11111111\n'\ + 'node1\treadonly\t0\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n' + assert distributed.query(query, settings={"max_memory_usage": 40000000, "readonly": 2}) == 'node1\tmax_memory_usage\t40000000\n'\ + 'node1\treadonly\t2\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n' + assert distributed.query(query, settings={"max_memory_usage": 3000000000, "readonly": 2}) == 'node1\tmax_memory_usage\t99999999\n'\ + 'node1\treadonly\t2\n'\ + 'node2\tmax_memory_usage\t10000000000\n'\ + 'node2\treadonly\t1\n'