V2.0.9 var array review #2521

renecannao · 2020-02-11T01:44:15Z

Creating a pull request to for review purposes. Not to be merged

lib/MySQL_Thread.cpp

renecannao · 2020-02-11T02:08:17Z

lib/MySQL_Thread.cpp

+	for (int i=0; i<SQL_NAME_LAST; i++) {
+		if (i == SQL_CHARACTER_SET || i == SQL_CHARACTER_SET_RESULTS ||
+			i == SQL_CHARACTER_SET_CONNECTION || i == SQL_CHARACTER_SET_CLIENT ||
+			i == SQL_CHARACTER_SET_DATABASE) {


All these variables cannot be set independently.

https://dev.mysql.com/doc/refman/8.0/en/charset-connection.html#charset-connection-client-configuration

We either:

only set collation_connection (what is that is actually exchanged in the protocol)

set collation_connection , and all the other variables need to be set accordingly

Default character set is used when backend mysql server dose not support configured collation/charset. For example, mysql 5.7 does not support collation is greater than 254. There for it might be configured to switch to the default character set.

This code is useless. I should not add it, because default values should not be used to initialize variables.

renecannao · 2020-02-11T02:11:16Z

lib/MySQL_Thread.cpp

+			ss << ci->nr;
+			sess->mysql_variables->client_set_value(i, ss.str());
+		} else {
+			sess->mysql_variables->client_set_value(i, mysql_thread___default_variables[i]);


For now, I am not sure about this.
If the client didn't set the variable, and a default it is not required (for example, collation_connection), probably we should not assign these variables.
To me, assigning a variable means that it needs to be tracked.

I may remove this comment after further review.

I have removed this code completely. Client character set initialization takes place in

bool MySQL_Protocol::process_pkt_handshake_response(unsigned char *pkt, unsigned int len)

From my understanding, default values, that are configured in the global_variables should not be used to initialize character set variables.

Correct.
As part of the handshake, in MySQL_Protocol::generate_pkt_initial_handshake() , we have this:

const MARIADB_CHARSET_INFO *ci = NULL; ci = proxysql_find_charset_name(mysql_thread___default_variables[SQL_CHARACTER_SET]); if (!ci) { proxy_error("Cannot find character set for name [%s]. Configuration error. Check [%s] global variable.\n", mysql_thread___default_variables[SQL_CHARACTER_SET], mysql_tracked_variables[SQL_CHARACTER_SET].internal_variable_name); assert(0); } uint8_t uint8_charset = ci->nr & 255;

The client can, in process_pkt_handshake_response, either:

accept it replying with the same collation id

accept it replying with value 0 see Crash on mysql_connection.cpp:417:set_names_start() #810

change it returning a different collation

renecannao · 2020-02-11T02:21:56Z

lib/MySQL_Thread.cpp

@@ -3683,7 +3341,7 @@ bool MySQL_Thread::init() {

 	match_regexes=(Session_Regex **)malloc(sizeof(Session_Regex *)*4);
 	match_regexes[0]=new Session_Regex((char *)"^SET (|SESSION |@@|@@session.)SQL_LOG_BIN( *)(:|)=( *)");
-	match_regexes[1]=new Session_Regex((char *)"^SET (|SESSION |@@|@@session.)(SQL_MODE|TIME_ZONE|CHARACTER_SET_RESULTS|SESSION_TRACK_GTIDS|SQL_AUTO_IS_NULL|SQL_SELECT_LIMIT|SQL_SAFE_UPDATES|COLLATION_CONNECTION|NET_WRITE_TIMEOUT|TX_ISOLATION|MAX_JOIN_SIZE( *)(:|)=( *))");
+	match_regexes[1]=new Session_Regex((char *)"^SET (|SESSION |@@|@@session.)(SQL_MODE|TIME_ZONE|CHARACTER_SET_RESULTS|CHARACTER_SET_CLIENT|CHARACTER_SET_DATABASE|SESSION_TRACK_GTIDS|SQL_AUTO_IS_NULL|SQL_SELECT_LIMIT|SQL_SAFE_UPDATES|COLLATION_CONNECTION|CHARACTER_SET_CONNECTION|NET_WRITE_TIMEOUT|TX_ISOLATION|MAX_JOIN_SIZE( *)(:|)=( *))");


I think we should automatically generate this regex, so we won't forget when adding new variables.

renecannao · 2020-02-11T02:29:23Z

lib/MySQL_Variables.cpp

+
+void print_backtrace(void);
+
+void MySQL_Variables::client_set_value(int idx, const char* value) {


This function doesn't check if the client exists.

I see this function called by MySQL_Connection::set_charset() , that is in turned called by MySQL_Connection::connect_start() .
MySQL_Connection::connect_start() creates a connection to the backend.
Some sessions do not have a client connection, for example sessions used by mirroring.
Therefore, trying to set a variable on a not existing client will cause a crash

renecannao · 2020-02-11T02:38:18Z

lib/mysql_connection.cpp

-	set_charset(c->nr, NAMES);
+	proxy_warning("TRACE : INITIAL ACTION client %s, server %s\n", myds->sess->mysql_variables->client_get_value(SQL_CHARACTER_ACTION), myds->sess->mysql_variables->server_get_value(SQL_CHARACTER_ACTION));
+	set_charset(c->nr, (charset_action)atoi(myds->sess->mysql_variables->client_get_value(SQL_CHARACTER_ACTION)));
+	//set_charset(c->nr, CONNECT_START);
 	mysql_options(mysql, MYSQL_SET_CHARSET_NAME, c->csname);


Note to myself: the client (proxysql) send to the server (the backend) the default collation for a specific charset).

https://dev.mysql.com/doc/refman/8.0/en/charset-connection.html#charset-connection-client-configuration

If the client is using a collation that is not the default, we may need to change it later on.
We can probably force the desired collation directly into the struct mysql , instead of using mysql_options

pondix · 2020-02-16T17:33:11Z

This causes the automated test server to crash as ProxySQL just accumulates memory until it crashes :D try avoid submitting for build testing until it works...

V2.0.9 web2

Fix LOAD MYSQL SERVERS FROM CONFIG

Fix galera query in maint_mode

Better handling of transactions in SQLite3

use DIGEST_STATS_FAST_1 always

Revert to auto configuration of build job num

adding MariaDB ConnectorJ test case to set parser tests

- SQL_SAFE_UPDATES - SQL_SELECT_LIMIT - SQL_SQL_MODE - automation tests

…n test

Added new array mysql_tracked_variables[] that defines the tracked variables. Ideally, tracking a new session variable will requires adding only a couple of lines of code. Removed session variables from mysql_thread_variables_names: - now both MySQL_Threads_Handler::get_variables_list() and MySQL_Threads_Handler::has_variable() rely on mysql_tracked_variables[] Simplified MySQL_Variables, as it now also uses mysql_tracked_variables[] .

Generalized code to handle two variables together: - sql_auto_is_null - sql_safe_updates

- collation_connection - net_write_timeout - max_join_size

pondix · 2022-03-10T21:50:31Z

Automated message: PR pending admin approval for build testing

renecannao added 3 commits February 7, 2020 16:00

Preparing skelethon for web ui

d266e29

Preparing the API for web interface plugin

238aeb8

class Web_Interface_plugin friends Query_Processor

c4607d9

renecannao requested a review from alpes214 February 11, 2020 01:44

renecannao commented Feb 11, 2020

View reviewed changes

lib/MySQL_Thread.cpp Show resolved Hide resolved

renecannao commented Feb 11, 2020

View reviewed changes

alpes214 force-pushed the v2.0.9-var-array-review branch 2 times, most recently from 5e7fcc4 to fd02a36 Compare February 14, 2020 06:50

Valentin Rakush and others added 17 commits February 17, 2020 09:55

added configuration data from web interface plugin testing

bc65cee

Changing version in Makefile

b3e9459

revert previous commit

9bdab2e

Merge pull request #2531 from sysown/v2.0.9-web2

7ee01a0

V2.0.9 web2

Empty commit

332059a

Better handling of transactions in SQLite3

5a84277

Fix LOAD MYSQL SERVERS FROM CONFIG

6b2072d

Fix galera query in maint_mode

12e61b8

Merge pull request #2538 from sysown/v2.0.10-fix-load-config

9c137df

Fix LOAD MYSQL SERVERS FROM CONFIG

Merge pull request #2539 from sysown/v2.0.10-fix-galera-query

0033767

Fix galera query in maint_mode

Merge pull request #2542 from sysown/v2.0.10-sqlite_server_test

13653c7

Better handling of transactions in SQLite3

use DIGEST_STATS_FAST_1 always

a095ef0

Merge pull request #2545 from sysown/v2.0.10-stats-fast

7e1901c

use DIGEST_STATS_FAST_1 always

Code cleanup

865b96d

Code cleanup

0ae253a

Implementation of OFFLINE_SOFT for pxc_maint_mode

2bdf044

memory alloc/free

a1d9c5a

renecannao and others added 25 commits February 29, 2020 09:43

Merge pull request #2571 from sysown/jobs-patch-1

d14fc52

Revert to auto configuration of build job num

Merge pull request #2567 from sysown/v2.0.11-2523

5c4e38b

adding MariaDB ConnectorJ test case to set parser tests

SQL_SAFE_UPDATES implementation in session variables array

5c5c7a0

Generic variables handling

107ebb6

- SQL_SAFE_UPDATES - SQL_SELECT_LIMIT - SQL_SQL_MODE - automation tests

SQL_TIME_ZONE

a58014b

CHARACTER_SET_RESULTS

63952da

SQL_TRANSACTION_ISOLATION

64cb514

SQL_TRANSACTION_READ

5a9fb4b

FIX: use isolation_level in internal session. use transaction_level i…

4b1bba9

…n test

FIX: tx_isolation uses data strictures for transaction isolataion level

bbad05e

SQL_SESSION_TRACK_GTIDS

88e654c

SQL_SQL_AUTO_IS_NULL

1e72e44

Make more use of mysql_tracked_variables[]

e305bb3

Code cleanup

1f537e8

Simplify parser in MySQL_Session

9cfa3b6

Generalized code to handle two variables together: - sql_auto_is_null - sql_safe_updates

Migrating more session variables to new algorithm

d5445c7

- collation_connection - net_write_timeout - max_join_size

Migrating more session variables to new algorithm

c79f374

- collation_connection - net_write_timeout - max_join_size

Code cleanup

e0dd17e

Close cleanup

f5e383f

fix

3da670d

adjust default values in the test

14ca80b

adjust settings for time_zone and sql_safe_updates

5ab69be

fix sql_select_limit

a92ddbf

Fix for isolation level

cba6285

alpes214 force-pushed the v2.0.9-var-array-review branch from 68df53b to 5ad09af Compare March 1, 2020 19:44

Character set handling implementation

0bb82bb

alpes214 force-pushed the v2.0.9-var-array-review branch from 5ad09af to 0bb82bb Compare March 2, 2020 19:04

renecannao closed this Jan 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V2.0.9 var array review #2521

V2.0.9 var array review #2521

renecannao commented Feb 11, 2020

renecannao Feb 11, 2020

alpes214 Feb 11, 2020

renecannao Feb 11, 2020

alpes214 Feb 11, 2020

renecannao Feb 11, 2020

renecannao Feb 11, 2020

renecannao Feb 11, 2020

alpes214 Feb 11, 2020

renecannao Feb 11, 2020

pondix commented Feb 16, 2020

pondix commented Mar 10, 2022


		void print_backtrace(void);

		void MySQL_Variables::client_set_value(int idx, const char* value) {

V2.0.9 var array review #2521

V2.0.9 var array review #2521

Conversation

renecannao commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pondix commented Feb 16, 2020

pondix commented Mar 10, 2022