From eb909a5db4ea844f69afb146c20de33ce25a01ff Mon Sep 17 00:00:00 2001 From: slvrtrn Date: Sat, 27 Jan 2024 18:36:38 +0100 Subject: [PATCH 1/4] Reapply add MySQL net write/read settings --- src/Server/MySQLHandler.cpp | 46 +++++++++++++------ src/Server/MySQLHandler.h | 9 ++-- .../02967_mysql_settings_override.reference | 23 ++++++++++ .../02967_mysql_settings_override.sh | 31 +++++++++++++ 4 files changed, 91 insertions(+), 18 deletions(-) create mode 100644 tests/queries/0_stateless/02967_mysql_settings_override.reference create mode 100755 tests/queries/0_stateless/02967_mysql_settings_override.sh diff --git a/src/Server/MySQLHandler.cpp b/src/Server/MySQLHandler.cpp index c159a09c874..260219c1556 100644 --- a/src/Server/MySQLHandler.cpp +++ b/src/Server/MySQLHandler.cpp @@ -66,7 +66,7 @@ static String showCountWarningsReplacementQuery(const String & query); static String selectEmptyReplacementQuery(const String & query); static String showTableStatusReplacementQuery(const String & query); static String killConnectionIdReplacementQuery(const String & query); -static String selectLimitReplacementQuery(const String & query); +static std::optional setSettingReplacementQuery(const String & query, const String & mysql_setting, const String & native_setting); MySQLHandler::MySQLHandler( IServer & server_, @@ -88,12 +88,14 @@ MySQLHandler::MySQLHandler( if (ssl_enabled) server_capabilities |= CLIENT_SSL; - replacements.emplace("SHOW WARNINGS", showWarningsReplacementQuery); - replacements.emplace("SHOW COUNT(*) WARNINGS", showCountWarningsReplacementQuery); - replacements.emplace("KILL QUERY", killConnectionIdReplacementQuery); - replacements.emplace("SHOW TABLE STATUS LIKE", showTableStatusReplacementQuery); - replacements.emplace("SHOW VARIABLES", selectEmptyReplacementQuery); - replacements.emplace("SET SQL_SELECT_LIMIT", selectLimitReplacementQuery); + queries_replacements.emplace("SHOW WARNINGS", showWarningsReplacementQuery); + queries_replacements.emplace("SHOW COUNT(*) WARNINGS", showCountWarningsReplacementQuery); + queries_replacements.emplace("KILL QUERY", killConnectionIdReplacementQuery); + queries_replacements.emplace("SHOW TABLE STATUS LIKE", showTableStatusReplacementQuery); + queries_replacements.emplace("SHOW VARIABLES", selectEmptyReplacementQuery); + settings_replacements.emplace("SQL_SELECT_LIMIT", "limit"); + settings_replacements.emplace("NET_WRITE_TIMEOUT", "send_timeout"); + settings_replacements.emplace("NET_READ_TIMEOUT", "receive_timeout"); } void MySQLHandler::run() @@ -342,16 +344,30 @@ void MySQLHandler::comQuery(ReadBuffer & payload, bool binary_protocol) bool should_replace = false; bool with_output = false; - for (auto const & x : replacements) + // Queries replacements + for (auto const & [query_to_replace, replacement_fn] : queries_replacements) { - if (0 == strncasecmp(x.first.c_str(), query.c_str(), x.first.size())) + if (0 == strncasecmp(query_to_replace.c_str(), query.c_str(), query_to_replace.size())) { should_replace = true; - replacement_query = x.second(query); + replacement_query = replacement_fn(query); break; } } + // Settings replacements + if (!should_replace) + for (auto const & [mysql_setting, native_setting] : settings_replacements) + { + const auto replacement_query_opt = setSettingReplacementQuery(query, mysql_setting, native_setting); + if (replacement_query_opt.has_value()) + { + should_replace = true; + replacement_query = replacement_query_opt.value(); + break; + } + } + ReadBufferFromString replacement(replacement_query); auto query_context = session->makeQueryContext(); @@ -601,12 +617,12 @@ static String showTableStatusReplacementQuery(const String & query) return query; } -static String selectLimitReplacementQuery(const String & query) +static std::optional setSettingReplacementQuery(const String & query, const String & mysql_setting, const String & native_setting) { - const String prefix = "SET SQL_SELECT_LIMIT"; - if (query.starts_with(prefix)) - return "SET limit" + std::string(query.data() + prefix.length()); - return query; + const String prefix = "SET " + mysql_setting; + if (0 == strncasecmp(prefix.c_str(), query.c_str(), prefix.size())) + return "SET " + native_setting + String(query.data() + prefix.length()); + return std::nullopt; } /// Replace "KILL QUERY [connection_id]" into "KILL QUERY WHERE query_id LIKE 'mysql:[connection_id]:xxx'". diff --git a/src/Server/MySQLHandler.h b/src/Server/MySQLHandler.h index 867a90a6205..8f9dcd872db 100644 --- a/src/Server/MySQLHandler.h +++ b/src/Server/MySQLHandler.h @@ -92,9 +92,12 @@ protected: MySQLProtocol::PacketEndpointPtr packet_endpoint; std::unique_ptr session; - using ReplacementFn = std::function; - using Replacements = std::unordered_map; - Replacements replacements; + using QueryReplacementFn = std::function; + using QueriesReplacements = std::unordered_map; + QueriesReplacements queries_replacements; + + using SettingsReplacements = std::unordered_map; + SettingsReplacements settings_replacements; std::mutex prepared_statements_mutex; UInt32 current_prepared_statement_id TSA_GUARDED_BY(prepared_statements_mutex) = 0; diff --git a/tests/queries/0_stateless/02967_mysql_settings_override.reference b/tests/queries/0_stateless/02967_mysql_settings_override.reference new file mode 100644 index 00000000000..bc058f4889e --- /dev/null +++ b/tests/queries/0_stateless/02967_mysql_settings_override.reference @@ -0,0 +1,23 @@ +-- Init +s +a +b +c +d +-- Uppercase tests +s +a +b +name value +send_timeout 22 +name value +receive_timeout 33 +-- Lowercase tests +s +a +b +c +name value +send_timeout 55 +name value +receive_timeout 66 diff --git a/tests/queries/0_stateless/02967_mysql_settings_override.sh b/tests/queries/0_stateless/02967_mysql_settings_override.sh new file mode 100755 index 00000000000..e3439a80ab4 --- /dev/null +++ b/tests/queries/0_stateless/02967_mysql_settings_override.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# Tags: no-fasttest +# Tag no-fasttest: requires mysql client + +# Tests the override of certain MySQL proprietary settings to ClickHouse native settings + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +CHANGED_SETTINGS_QUERY="SELECT name, value FROM system.settings WHERE name IN ('send_timeout', 'receive_timeout') AND changed;" + +TEST_TABLE="mysql_settings_override_test" + +DROP_TABLE="DROP TABLE IF EXISTS $TEST_TABLE;" +CREATE_TABLE="CREATE TABLE $TEST_TABLE (s String) ENGINE MergeTree ORDER BY s;" +INSERT_STMT="INSERT INTO $TEST_TABLE VALUES ('a'), ('b'), ('c'), ('d');" +SELECT_STMT="SELECT * FROM $TEST_TABLE;" + +echo "-- Init" +${MYSQL_CLIENT} --execute "$DROP_TABLE $CREATE_TABLE $INSERT_STMT $SELECT_STMT" # should fetch all 4 records + +echo "-- Uppercase tests" +${MYSQL_CLIENT} --execute "SET SQL_SELECT_LIMIT = 2; $SELECT_STMT" # should fetch 2 records out of 4 +${MYSQL_CLIENT} --execute "SET NET_WRITE_TIMEOUT = 22; $CHANGED_SETTINGS_QUERY" +${MYSQL_CLIENT} --execute "SET NET_READ_TIMEOUT = 33; $CHANGED_SETTINGS_QUERY" + +echo "-- Lowercase tests" +${MYSQL_CLIENT} --execute "set sql_select_limit=3; $SELECT_STMT" # should fetch 3 records out of 4 +${MYSQL_CLIENT} --execute "set net_write_timeout=55; $CHANGED_SETTINGS_QUERY" +${MYSQL_CLIENT} --execute "set net_read_timeout=66; $CHANGED_SETTINGS_QUERY" From 7d785c47e292b8f28d7c1b947393432316cd9c14 Mon Sep 17 00:00:00 2001 From: slvrtrn Date: Sat, 27 Jan 2024 18:45:57 +0100 Subject: [PATCH 2/4] Add drop table --- tests/queries/0_stateless/02967_mysql_settings_override.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/queries/0_stateless/02967_mysql_settings_override.sh b/tests/queries/0_stateless/02967_mysql_settings_override.sh index e3439a80ab4..cee18255eeb 100755 --- a/tests/queries/0_stateless/02967_mysql_settings_override.sh +++ b/tests/queries/0_stateless/02967_mysql_settings_override.sh @@ -29,3 +29,5 @@ echo "-- Lowercase tests" ${MYSQL_CLIENT} --execute "set sql_select_limit=3; $SELECT_STMT" # should fetch 3 records out of 4 ${MYSQL_CLIENT} --execute "set net_write_timeout=55; $CHANGED_SETTINGS_QUERY" ${MYSQL_CLIENT} --execute "set net_read_timeout=66; $CHANGED_SETTINGS_QUERY" + +${MYSQL_CLIENT} --execute "$DROP_TABLE" From 7cef679f6d4addb98017625065a3a7479d7f16c9 Mon Sep 17 00:00:00 2001 From: slvrtrn Date: Mon, 29 Jan 2024 20:11:02 +0100 Subject: [PATCH 3/4] Address PR feedback --- src/Server/MySQLHandler.cpp | 221 +++++++++--------- src/Server/MySQLHandler.h | 1 + .../02967_mysql_settings_override.sh | 8 +- 3 files changed, 116 insertions(+), 114 deletions(-) diff --git a/src/Server/MySQLHandler.cpp b/src/Server/MySQLHandler.cpp index 260219c1556..72fe3b7cea9 100644 --- a/src/Server/MySQLHandler.cpp +++ b/src/Server/MySQLHandler.cpp @@ -57,16 +57,109 @@ namespace ErrorCodes extern const int UNSUPPORTED_METHOD; } - static const size_t PACKET_HEADER_SIZE = 4; static const size_t SSL_REQUEST_PAYLOAD_SIZE = 32; -static String showWarningsReplacementQuery(const String & query); -static String showCountWarningsReplacementQuery(const String & query); -static String selectEmptyReplacementQuery(const String & query); -static String showTableStatusReplacementQuery(const String & query); -static String killConnectionIdReplacementQuery(const String & query); -static std::optional setSettingReplacementQuery(const String & query, const String & mysql_setting, const String & native_setting); +static bool checkShouldReplaceQuery(const String & query, const String & prefix) +{ + return query.length() >= prefix.length() + && std::equal(prefix.begin(), prefix.end(), query.begin(), [](char a, char b) { return std::tolower(a) == std::tolower(b); }); +} + +static bool isFederatedServerSetupSetCommand(const String & query) +{ + re2::RE2::Options regexp_options; + regexp_options.set_case_sensitive(false); + static const re2::RE2 expr( + "(^(SET NAMES(.*)))" + "|(^(SET character_set_results(.*)))" + "|(^(SET FOREIGN_KEY_CHECKS(.*)))" + "|(^(SET AUTOCOMMIT(.*)))" + "|(^(SET sql_mode(.*)))" + "|(^(SET @@(.*)))" + "|(^(SET SESSION TRANSACTION ISOLATION LEVEL(.*)))", regexp_options); + assert(expr.ok()); + return re2::RE2::FullMatch(query, expr); +} + +/// Always return an empty set with appropriate column definitions for SHOW WARNINGS queries +/// See also: https://dev.mysql.com/doc/refman/8.0/en/show-warnings.html +static String showWarningsReplacementQuery([[maybe_unused]] const String & query) +{ + return "SELECT '' AS Level, 0::UInt32 AS Code, '' AS Message WHERE false"; +} + +static String showCountWarningsReplacementQuery([[maybe_unused]] const String & query) +{ + return "SELECT 0::UInt64 AS `@@session.warning_count`"; +} + +/// Replace "[query(such as SHOW VARIABLES...)]" into "". +static String selectEmptyReplacementQuery(const String & query) +{ + std::ignore = query; + return "select ''"; +} + +/// Replace "SHOW TABLE STATUS LIKE 'xx'" into "SELECT ... FROM system.tables WHERE name LIKE 'xx'". +static String showTableStatusReplacementQuery(const String & query) +{ + const String prefix = "SHOW TABLE STATUS LIKE "; + if (query.size() > prefix.size()) + { + String suffix = query.data() + prefix.length(); + return ( + "SELECT" + " name AS Name," + " engine AS Engine," + " '10' AS Version," + " 'Dynamic' AS Row_format," + " 0 AS Rows," + " 0 AS Avg_row_length," + " 0 AS Data_length," + " 0 AS Max_data_length," + " 0 AS Index_length," + " 0 AS Data_free," + " 'NULL' AS Auto_increment," + " metadata_modification_time AS Create_time," + " metadata_modification_time AS Update_time," + " metadata_modification_time AS Check_time," + " 'utf8_bin' AS Collation," + " 'NULL' AS Checksum," + " '' AS Create_options," + " '' AS Comment" + " FROM system.tables" + " WHERE name LIKE " + + suffix); + } + return query; +} + +static std::optional setSettingReplacementQuery(const String & query, const String & mysql_setting, const String & clickhouse_setting) +{ + const String prefix = "SET " + mysql_setting; + // if (query.length() >= prefix.length() && boost::iequals(std::string_view(prefix), std::string_view(query.data(), 3))) + if (checkShouldReplaceQuery(query, prefix)) + return "SET " + clickhouse_setting + String(query.data() + prefix.length()); + return std::nullopt; +} + +/// Replace "KILL QUERY [connection_id]" into "KILL QUERY WHERE query_id LIKE 'mysql:[connection_id]:xxx'". +static String killConnectionIdReplacementQuery(const String & query) +{ + const String prefix = "KILL QUERY "; + if (query.size() > prefix.size()) + { + String suffix = query.data() + prefix.length(); + static const re2::RE2 expr("^[0-9]"); + if (re2::RE2::FullMatch(suffix, expr)) + { + String replacement = fmt::format("KILL QUERY WHERE query_id LIKE 'mysql:{}:%'", suffix); + return replacement; + } + } + return query; +} MySQLHandler::MySQLHandler( IServer & server_, @@ -326,8 +419,6 @@ void MySQLHandler::comPing() packet_endpoint->sendPacket(OKPacket(0x0, client_capabilities, 0, 0, 0), true); } -static bool isFederatedServerSetupSetCommand(const String & query); - void MySQLHandler::comQuery(ReadBuffer & payload, bool binary_protocol) { String query = String(payload.position(), payload.buffer().end()); @@ -347,7 +438,7 @@ void MySQLHandler::comQuery(ReadBuffer & payload, bool binary_protocol) // Queries replacements for (auto const & [query_to_replace, replacement_fn] : queries_replacements) { - if (0 == strncasecmp(query_to_replace.c_str(), query.c_str(), query_to_replace.size())) + if (checkShouldReplaceQuery(query, query_to_replace)) { should_replace = true; replacement_query = replacement_fn(query); @@ -357,9 +448,9 @@ void MySQLHandler::comQuery(ReadBuffer & payload, bool binary_protocol) // Settings replacements if (!should_replace) - for (auto const & [mysql_setting, native_setting] : settings_replacements) + for (auto const & [mysql_setting, clickhouse_setting] : settings_replacements) { - const auto replacement_query_opt = setSettingReplacementQuery(query, mysql_setting, native_setting); + const auto replacement_query_opt = setSettingReplacementQuery(query, mysql_setting, clickhouse_setting); if (replacement_query_opt.has_value()) { should_replace = true; @@ -368,8 +459,6 @@ void MySQLHandler::comQuery(ReadBuffer & payload, bool binary_protocol) } } - ReadBufferFromString replacement(replacement_query); - auto query_context = session->makeQueryContext(); query_context->setCurrentQueryId(fmt::format("mysql:{}:{}", connection_id, toString(UUIDHelpers::generateV4()))); CurrentThread::QueryScope query_scope{query_context}; @@ -401,7 +490,14 @@ void MySQLHandler::comQuery(ReadBuffer & payload, bool binary_protocol) } }; - executeQuery(should_replace ? replacement : payload, *out, false, query_context, set_result_details, QueryFlags{}, format_settings); + if (should_replace) + { + ReadBufferFromString replacement(replacement_query); + executeQuery(replacement, *out, false, query_context, set_result_details, QueryFlags{}, format_settings); + } + else + executeQuery(payload, *out, false, query_context, set_result_details, QueryFlags{}, format_settings); + if (!with_output) packet_endpoint->sendPacket(OKPacket(0x00, client_capabilities, affected_rows, 0, 0), true); @@ -547,99 +643,4 @@ void MySQLHandlerSSL::finishHandshakeSSL( } #endif - -static bool isFederatedServerSetupSetCommand(const String & query) -{ - re2::RE2::Options regexp_options; - regexp_options.set_case_sensitive(false); - static const re2::RE2 expr( - "(^(SET NAMES(.*)))" - "|(^(SET character_set_results(.*)))" - "|(^(SET FOREIGN_KEY_CHECKS(.*)))" - "|(^(SET AUTOCOMMIT(.*)))" - "|(^(SET sql_mode(.*)))" - "|(^(SET @@(.*)))" - "|(^(SET SESSION TRANSACTION ISOLATION LEVEL(.*)))", regexp_options); - assert(expr.ok()); - return re2::RE2::FullMatch(query, expr); -} - -/// Always return an empty set with appropriate column definitions for SHOW WARNINGS queries -/// See also: https://dev.mysql.com/doc/refman/8.0/en/show-warnings.html -static String showWarningsReplacementQuery([[maybe_unused]] const String & query) -{ - return "SELECT '' AS Level, 0::UInt32 AS Code, '' AS Message WHERE false"; -} - -static String showCountWarningsReplacementQuery([[maybe_unused]] const String & query) -{ - return "SELECT 0::UInt64 AS `@@session.warning_count`"; -} - -/// Replace "[query(such as SHOW VARIABLES...)]" into "". -static String selectEmptyReplacementQuery(const String & query) -{ - std::ignore = query; - return "select ''"; -} - -/// Replace "SHOW TABLE STATUS LIKE 'xx'" into "SELECT ... FROM system.tables WHERE name LIKE 'xx'". -static String showTableStatusReplacementQuery(const String & query) -{ - const String prefix = "SHOW TABLE STATUS LIKE "; - if (query.size() > prefix.size()) - { - String suffix = query.data() + prefix.length(); - return ( - "SELECT" - " name AS Name," - " engine AS Engine," - " '10' AS Version," - " 'Dynamic' AS Row_format," - " 0 AS Rows," - " 0 AS Avg_row_length," - " 0 AS Data_length," - " 0 AS Max_data_length," - " 0 AS Index_length," - " 0 AS Data_free," - " 'NULL' AS Auto_increment," - " metadata_modification_time AS Create_time," - " metadata_modification_time AS Update_time," - " metadata_modification_time AS Check_time," - " 'utf8_bin' AS Collation," - " 'NULL' AS Checksum," - " '' AS Create_options," - " '' AS Comment" - " FROM system.tables" - " WHERE name LIKE " - + suffix); - } - return query; -} - -static std::optional setSettingReplacementQuery(const String & query, const String & mysql_setting, const String & native_setting) -{ - const String prefix = "SET " + mysql_setting; - if (0 == strncasecmp(prefix.c_str(), query.c_str(), prefix.size())) - return "SET " + native_setting + String(query.data() + prefix.length()); - return std::nullopt; -} - -/// Replace "KILL QUERY [connection_id]" into "KILL QUERY WHERE query_id LIKE 'mysql:[connection_id]:xxx'". -static String killConnectionIdReplacementQuery(const String & query) -{ - const String prefix = "KILL QUERY "; - if (query.size() > prefix.size()) - { - String suffix = query.data() + prefix.length(); - static const re2::RE2 expr("^[0-9]"); - if (re2::RE2::FullMatch(suffix, expr)) - { - String replacement = fmt::format("KILL QUERY WHERE query_id LIKE 'mysql:{}:%'", suffix); - return replacement; - } - } - return query; -} - } diff --git a/src/Server/MySQLHandler.h b/src/Server/MySQLHandler.h index 8f9dcd872db..2deb2b8f435 100644 --- a/src/Server/MySQLHandler.h +++ b/src/Server/MySQLHandler.h @@ -96,6 +96,7 @@ protected: using QueriesReplacements = std::unordered_map; QueriesReplacements queries_replacements; + /// MySQL setting name --> ClickHouse setting name using SettingsReplacements = std::unordered_map; SettingsReplacements settings_replacements; diff --git a/tests/queries/0_stateless/02967_mysql_settings_override.sh b/tests/queries/0_stateless/02967_mysql_settings_override.sh index cee18255eeb..59a2099190a 100755 --- a/tests/queries/0_stateless/02967_mysql_settings_override.sh +++ b/tests/queries/0_stateless/02967_mysql_settings_override.sh @@ -2,7 +2,7 @@ # Tags: no-fasttest # Tag no-fasttest: requires mysql client -# Tests the override of certain MySQL proprietary settings to ClickHouse native settings +# Tests that certain MySQL-proprietary settings are mapped to ClickHouse-native settings. CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh @@ -15,17 +15,17 @@ TEST_TABLE="mysql_settings_override_test" DROP_TABLE="DROP TABLE IF EXISTS $TEST_TABLE;" CREATE_TABLE="CREATE TABLE $TEST_TABLE (s String) ENGINE MergeTree ORDER BY s;" INSERT_STMT="INSERT INTO $TEST_TABLE VALUES ('a'), ('b'), ('c'), ('d');" -SELECT_STMT="SELECT * FROM $TEST_TABLE;" +SELECT_STMT="SELECT * FROM $TEST_TABLE ORDER BY s;" echo "-- Init" ${MYSQL_CLIENT} --execute "$DROP_TABLE $CREATE_TABLE $INSERT_STMT $SELECT_STMT" # should fetch all 4 records -echo "-- Uppercase tests" +echo "-- Uppercase setting name" ${MYSQL_CLIENT} --execute "SET SQL_SELECT_LIMIT = 2; $SELECT_STMT" # should fetch 2 records out of 4 ${MYSQL_CLIENT} --execute "SET NET_WRITE_TIMEOUT = 22; $CHANGED_SETTINGS_QUERY" ${MYSQL_CLIENT} --execute "SET NET_READ_TIMEOUT = 33; $CHANGED_SETTINGS_QUERY" -echo "-- Lowercase tests" +echo "-- Lowercase setting name" ${MYSQL_CLIENT} --execute "set sql_select_limit=3; $SELECT_STMT" # should fetch 3 records out of 4 ${MYSQL_CLIENT} --execute "set net_write_timeout=55; $CHANGED_SETTINGS_QUERY" ${MYSQL_CLIENT} --execute "set net_read_timeout=66; $CHANGED_SETTINGS_QUERY" From 5192d383705510e6b4de0016bd475b0582f48b5e Mon Sep 17 00:00:00 2001 From: slvrtrn Date: Mon, 29 Jan 2024 20:35:15 +0100 Subject: [PATCH 4/4] Fix tests --- .../0_stateless/02967_mysql_settings_override.reference | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/queries/0_stateless/02967_mysql_settings_override.reference b/tests/queries/0_stateless/02967_mysql_settings_override.reference index bc058f4889e..96cf7ecc403 100644 --- a/tests/queries/0_stateless/02967_mysql_settings_override.reference +++ b/tests/queries/0_stateless/02967_mysql_settings_override.reference @@ -4,7 +4,7 @@ a b c d --- Uppercase tests +-- Uppercase setting name s a b @@ -12,7 +12,7 @@ name value send_timeout 22 name value receive_timeout 33 --- Lowercase tests +-- Lowercase setting name s a b