Merge pull request #3596 from sysown/v2.2.1-3595

Closes #3595: Backport bugfixes to v2.2.1 - 1

Author: JavierJF

include/MySQL_Session.h

@@ -79,7 +79,18 @@ class MySQL_Session
 
 	void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_FIELD_LIST(PtrSize_t *);
 	void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_INIT_DB(PtrSize_t *);
-	void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_USE_DB(PtrSize_t *);
+	/**
+	 * @brief Handles 'COM_QUERIES' holding 'USE DB' statements.
+	 *
+	 * @param pkt The packet being processed.
+	 * @param query_digest The query digest returned by the 'QueryProcessor'
+	 *   holding the 'USE' statement without the initial comment.
+	 *
+	 * @details NOTE: This function used to be called from 'handler_special_queries'.
+	 *   But since it was change for handling 'USE' statements which are preceded by
+	 *   comments, it's called after 'QueryProcessor' has processed the query.
+	 */
+	void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_USE_DB(PtrSize_t *pkt, const char* query_digest);
 	void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_PING(PtrSize_t *);
 
 	void handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_CHANGE_USER(PtrSize_t *, bool *);

lib/MySQL_Session.cpp

@@ -1135,12 +1135,6 @@ bool MySQL_Session::handler_special_queries(PtrSize_t *pkt) {
 		}
 	}
 
-	if (session_type != PROXYSQL_SESSION_CLICKHOUSE) {
-		if (pkt->size>(5+4) && strncasecmp((char *)"USE ",(char *)pkt->ptr+5,4)==0) {
-			handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_USE_DB(pkt);
-			return true;
-		}
-	}
 	if (pkt->size==SELECT_VERSION_COMMENT_LEN+5 && strncmp((char *)SELECT_VERSION_COMMENT,(char *)pkt->ptr+5,pkt->size-5)==0) {
 		// FIXME: this doesn't return AUTOCOMMIT or IN_TRANS
 		PtrSize_t pkt_2;
@@ -3593,6 +3587,23 @@ int MySQL_Session::get_pkts_from_client(bool& wrong_pass, PtrSize_t& pkt) {
 										clock_gettime(CLOCK_THREAD_CPUTIME_ID,&begint);
 									}
 									qpo=GloQPro->process_mysql_query(this,pkt.ptr,pkt.size,&CurrentQuery);
+									// This block was moved from 'handler_special_queries' to support
+									// handling of 'USE' statements which are preceded by a comment.
+									// For more context check issue: #3493.
+									// ===================================================
+									if (session_type != PROXYSQL_SESSION_CLICKHOUSE) {
+										if (strncasecmp((char *)"USE ",CurrentQuery.get_digest_text(),4)==0) {
+											handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_USE_DB(&pkt, CurrentQuery.get_digest_text());
+
+											if (mirror == false) {
+												break;
+											} else {
+												handler_ret = -1;
+												return handler_ret;
+											}
+										}
+									}
+									// ===================================================
 									if (qpo->max_lag_ms >= 0) {
 										thread->status_variables.stvar[st_var_queries_with_max_lag_ms]++;
 									}
@@ -5219,12 +5230,12 @@ void MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
 
 // this function was introduced due to isseu #718
 // some application (like the one written in Perl) do not use COM_INIT_DB , but COM_QUERY with USE dbname
-void MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_USE_DB(PtrSize_t *pkt) {
+void MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_USE_DB(PtrSize_t *pkt, const char* query_digest) {
 	gtid_hid=-1;
 	proxy_debug(PROXY_DEBUG_MYSQL_COM, 5, "Got COM_QUERY with USE dbname\n");
 	if (session_type == PROXYSQL_SESSION_MYSQL) {
 		__sync_fetch_and_add(&MyHGM->status.frontend_use_db, 1);
-		char *schemaname=strndup((char *)pkt->ptr+sizeof(mysql_hdr)+5,pkt->size-sizeof(mysql_hdr)-5);
+		char *schemaname=strndup(query_digest+4,strlen(query_digest)-4);
 		char *schemanameptr=trim_spaces_and_quotes_in_place(schemaname);
 		// handle cases like "USE `schemaname`
 		if(schemanameptr[0]=='`' && schemanameptr[strlen(schemanameptr)-1]=='`') {
@@ -5443,7 +5454,11 @@ bool MySQL_Session::handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_C
 #endif
 			if (index(dig,';') && (index(dig,';') != dig + strlen(dig)-1)) {
 				string nqn = string((char *)CurrentQuery.QueryPointer,CurrentQuery.QueryLength);
-				proxy_warning("Unable to parse multi-statements command with SET statement: setting lock hostgroup . Command: %s\n", nqn.c_str());
+				proxy_warning(
+					"Unable to parse multi-statements command with SET statement from client"
+					" %s:%d: setting lock hostgroup. Command: %s\n", client_myds->addr.addr,
+					client_myds->addr.port, nqn.c_str()
+				);
 				*lock_hostgroup = true;
 				return false;
 			}

lib/MySQL_Thread.cpp

@@ -2542,7 +2542,7 @@ bool MySQL_Thread::init() {
 	match_regexes[0]=new Session_Regex((char *)"^SET (|SESSION |@@|@@session.)SQL_LOG_BIN( *)(:|)=( *)");
 
 	std::stringstream ss;
-	ss << "^SET (|SESSION |@@|@@session.)(" << mysql_variables.variables_regexp << "SESSION_TRACK_GTIDS|TX_ISOLATION( *)(:|)=( *))";
+	ss << "^SET (|SESSION |@@|@@session.)`?(" << mysql_variables.variables_regexp << "SESSION_TRACK_GTIDS|TX_ISOLATION)`?( *)(:|)=( *)";
 	match_regexes[1]=new Session_Regex((char *)ss.str().c_str());
 
 	match_regexes[2]=new Session_Regex((char *)"^SET(?: +)(|SESSION +)TRANSACTION(?: +)(?:(?:(ISOLATION(?: +)LEVEL)(?: +)(REPEATABLE(?: +)READ|READ(?: +)COMMITTED|READ(?: +)UNCOMMITTED|SERIALIZABLE))|(?:(READ)(?: +)(WRITE|ONLY)))");

lib/set_parser.cpp

@@ -34,7 +34,7 @@ std::map<std::string,std::vector<std::string>> SetParser::parse1() {
 	std::map<std::string,std::vector<std::string>> result;
 
 #define SESSION_P1 "(?:|SESSION +|@@|@@session.)"
-#define VAR_P1 "(@\\w+|\\w+)"
+#define VAR_P1 "`?(@\\w+|\\w+)`?"
 //#define VAR_VALUE "((?:[\\w/\\d:\\+\\-]|,)+)"
 //#define VAR_VALUE "((?:CONCAT\\((?:(REPLACE|CONCAT)\\()+@@sql_mode,(?:(?:'|\\w|,| |\"|\\))+(?:\\)))|(?:[@\\w/\\d:\\+\\-]|,)+|(?:)))"
 #define VAR_VALUE_P1 "((?:\\()*(?:SELECT)*(?: )*(?:CONCAT\\()*(?:(?:(?: )*REPLACE|IFNULL|CONCAT)\\()+(?: )*(?:NULL|@OLD_SQL_MODE|@@SQL_MODE),(?:(?:'|\\w|,| |\"|\\))+(?:\\))*)(?:\\))|(?:NULL)|(?:[@\\w/\\d:\\+\\-]|,)+|(?:(?:'{1}|\"{1})(?:)(?:'{1}|\"{1})))"

test/tap/tests/reg_test_3493-USE_with_comment-t.cpp

@@ -0,0 +1,106 @@
+/**
+ * @file reg_test_3493-USE_with_comment-t.cpp
+ * @brief This test verifies that a 'USE' statement is properly tracked by
+ *   ProxySQL, even when executed with a leading comment.
+ * @details For being sure that the feature is properly supported the
+ *   test performs the following actions:
+ *
+ *   1. Open a MYSQL connection to ProxySQL.
+ *   2. Drops and creates a new database called 'reg_test_3493_use_comment'.
+ *   3. Checks the currently selected database in **a new backend database
+ *      connection** by means of the connection annotation
+ *      "create_new_connection=1". This way it's ensured that ProxySQL is
+ *      properly keeping track of the database selected in the issued 'USE'
+ *      statement.
+ */
+
+#include <cstring>
+#include <vector>
+#include <string>
+#include <stdio.h>
+
+#include <mysql.h>
+
+#include "tap.h"
+#include "command_line.h"
+#include "utils.h"
+#include "json.hpp"
+
+using nlohmann::json;
+
+void parse_result_json_column(MYSQL_RES *result, json& j) {
+	if(!result) return;
+	MYSQL_ROW row;
+
+	while ((row = mysql_fetch_row(result))) {
+		j = json::parse(row[0]);
+	}
+}
+
+int main(int argc, char** argv) {
+	CommandLine cl;
+
+	if (cl.getEnv()) {
+		diag("Failed to get the required environmental variables.");
+		return -1;
+	}
+
+	MYSQL* proxysql_mysql = mysql_init(NULL);
+
+	if (
+		!mysql_real_connect(
+			proxysql_mysql, cl.host, cl.username, cl.password, NULL, cl.port, NULL, 0
+		)
+	) {
+		fprintf(
+			stderr, "File %s, line %d, Error: %s\n", __FILE__, __LINE__,
+			mysql_error(proxysql_mysql)
+		);
+		return EXIT_FAILURE;
+	}
+
+	// Prepare the DB for the test
+	MYSQL_QUERY(proxysql_mysql, "DROP DATABASE IF EXISTS reg_test_3493_use_comment");
+	MYSQL_QUERY(proxysql_mysql, "CREATE DATABASE reg_test_3493_use_comment");
+
+	int err = mysql_query(proxysql_mysql, "/*+ placeholder_comment */ USE reg_test_3493_use_comment");
+	if (err) {
+		diag(
+			"'USE' command failed with error code '%d' and error '%s'",
+			err, mysql_error(proxysql_mysql)
+		);
+		return EXIT_FAILURE;
+	}
+
+	// Perform the 'SELECT DATABASE()' query in a new backend connection, to
+	// verify that ProxySQL is properly tracking the previously performed 'USE'
+	// statement.
+	MYSQL_QUERY(proxysql_mysql, "/*+ ;create_new_connection=1 */ SELECT DATABASE()");
+	MYSQL_RES* result = mysql_store_result(proxysql_mysql);
+	if (result == nullptr) {
+		diag("Invalid 'MYSQL_RES' returned from 'SELECT DATABASE()'");
+		return EXIT_FAILURE;
+	}
+
+	MYSQL_ROW row = mysql_fetch_row(result);
+	if (row == nullptr) {
+		diag("Invalid 'MYSQL_ROW' returned from 'SELECT DATABASE()'");
+		return EXIT_FAILURE;
+	}
+
+	std::string database_name { row[0] };
+
+	ok(
+		database_name == "reg_test_3493_use_comment",
+		"Selected DB name should be equal to actual DB name: (Exp: '%s') == (Act: '%s')",
+		"reg_test_3493_use_comment",
+		database_name.c_str()
+	);
+
+	// Drop created database
+	MYSQL_QUERY(proxysql_mysql, "DROP DATABASE IF EXISTS reg_test_3493_use_comment");
+
+	mysql_close(proxysql_mysql);
+
+	return exit_status();
+}

test/tap/tests/set_testing-t.csv

@@ -92,3 +92,11 @@
 "SET time_zone='+04:00', sql_mode='NO_ENGINE_SUBSTITUTION', max_join_size=10000; SET CHARSET 'latin1'", "{'time_zone':'+04:00', 'sql_mode':'NO_ENGINE_SUBSTITUTION', 'max_join_size':'10000', 'character_set_results':'latin1', 'character_set_client':'latin1'}", "['character_set_connection', 'collation_connection']"
 "SET session_track_gtids=ALL_GTIDS", "{'session_track_gtids':'ALL_GTIDS'}"
 "SET sql_safe_updates=1, session_track_schema=1, sql_mode = concat(@@sql_mode,',STRICT_TRANS_TABLES')", "{'sql_safe_updates':'ON', 'sql_mode':'concat(@@sql_mode,\',STRICT_TRANS_TABLES\')'}"
+"set `group_concat_max_len`=4096", "{'group_concat_max_len':'4096'}"
+"SET `sql_select_limit`=3030; ", "{'sql_select_limit':'3030'}"
+"SET `time_zone`='+04:00', `sql_mode`='NO_ENGINE_SUBSTITUTION', `max_join_size`=10000; SET CHARACTER SET 'latin1'", "{'time_zone':'+04:00', 'sql_mode':'NO_ENGINE_SUBSTITUTION', 'max_join_size':'10000', 'character_set_results':'latin1', 'character_set_client':'latin1'}", "['character_set_connection', 'collation_connection']"
+"SET `character_set_results`='binary', `sql_mode`='NO_ENGINE_SUBSTITUTION', `character_set_client`='latin1', `max_join_size`=10000", "{'character_set_results':'binary', 'sql_mode':'NO_ENGINE_SUBSTITUTION', 'character_set_client':'latin1', 'max_join_size':'10000'}"
+"set `tx_isolation`='READ-COMMITTED', `group_concat_max_len`=4096", "{'transaction_isolation':'READ-COMMITTED', 'group_concat_max_len':'4096'}"
+"set `net_write_timeout`=30", "{'net_write_timeout':'30'}"
+"SET `sql_auto_is_null`=ON", "{'sql_auto_is_null':'ON'}"
+"set `character_set_results`=null; set names latin7; set `character_set_client`='utf8mb4';", "{'character_set_results':'latin7', 'collation_connection':'latin7_general_ci', 'character_set_connection':'latin7', 'character_set_client':'utf8mb4'}"

test/tap/tests/test_filtered_set_statements-t.cpp

@@ -48,6 +48,12 @@ std::vector<std::pair<std::string, std::string>> filtered_set_queries {
 	{ "net_read_timeout", "28801" },
 	// NOTE: This variable has been temporarily ignored. Check issues #3442 and #3441.
 	{ "session_track_schema", "1" },
+	// Added several variables to be set using `grave accents`. See issue #3479.
+	{ "`wait_timeout`", "28801" },
+	{ "`character_set_results`", "latin1" },
+	{ "`character_set_results`", "latin1" },
+	{ "`autocommit`", "1" },
+	{ "`max_join_size`", "18446744073709551615" },
 };
 
 std::vector<std::string> get_valid_set_query_set(const std::string& set_query, const std::string param) {