Re: [Maria-developers] [Commits] 9b88b52: MDEV-8931: (server part of) session state tracking

Hi, Sanja! Here's a combined review of all three patches: On May 30, Oleksandr Byelkin wrote:

diff --git a/include/mysql_com.h b/include/mysql_com.h index c13999a..954c173 100644 --- a/include/mysql_com.h +++ b/include/mysql_com.h @@ -520,6 +544,30 @@ enum enum_mysql_set_option MYSQL_OPTION_MULTI_STATEMENTS_OFF };

+/* + Type of state change information that the server can include in the Ok + packet. + Note : 1) session_state_type shouldn't go past 255 (i.e. 1-byte boundary). + 2) Modify the definition of SESSION_TRACK_END when a new member is + added.

these "notes" are a fragile way of adding restrictions. 1. add a compile_time_assert for SESSION_TRACK_END < 256 2. in fact, you have compile_time_assert's for SESSION_TRACK_SCHEMA and SESSION_TRACK_STATE_CHANGE, but they say < 251. And it looks like SESSION_TRACK_END should be < 251 (not 256) too. 3. perhaps it's better to move SESSION_TRACK_END into the enum, and give it a visually distinct name, like SESSION_TRACK_always_at_the_end then it'll always be the last, and you can compile_time_assert that it's less than 251.

+*/ +enum enum_session_state_type +{ + SESSION_TRACK_SYSTEM_VARIABLES, /* Session system variables */ + SESSION_TRACK_SCHEMA, /* Current schema */ + SESSION_TRACK_STATE_CHANGE, /* track session state changes */ + SESSION_TRACK_GTIDS, + SESSION_TRACK_TRANSACTION_CHARACTERISTICS, /* Transaction chistics */ + SESSION_TRACK_TRANSACTION_STATE /* Transaction state */ +}; + +#define SESSION_TRACK_BEGIN SESSION_TRACK_SYSTEM_VARIABLES + +#define SESSION_TRACK_END SESSION_TRACK_TRANSACTION_STATE + +#define IS_SESSION_STATE_TYPE(T) \ + (((int)(T) >= SESSION_TRACK_BEGIN) && ((T) <= SESSION_TRACK_END)) + #define net_new_transaction(net) ((net)->pkt_nr=0)

#ifdef __cplusplus diff --git a/libmysqld/lib_sql.cc b/libmysqld/lib_sql.cc index bcb45ae..c7ebb79 100644 --- a/libmysqld/lib_sql.cc +++ b/libmysqld/lib_sql.cc @@ -1171,7 +1171,8 @@ bool net_send_ok(THD *thd, uint server_status, uint statement_warn_count, ulonglong affected_rows, ulonglong id, const char *message, - bool unused __attribute__((unused))) + bool unused1 __attribute__((unused)), + bool unused2 __attribute__((unused)))

as I wrote in the previous review: 1. you don't need __attribute__((unused)) in C++, you can omit the parameter name 2. we compile with -Wno-unused-parameter anyway

{ DBUG_ENTER("emb_net_send_ok"); MYSQL_DATA *data; diff --git a/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result index cc43265..be6893c 100644 --- a/mysql-test/r/mysqld--help.result +++ b/mysql-test/r/mysqld--help.result @@ -903,6 +903,22 @@ The following options may be given as the first argument: files within specified directory --server-id=# Uniquely identifies the server instance in the community of replication partners + --session-track-schema + Track changes to the 'default schema'.

why in quotes?

+ (Defaults to on; use --skip-session-track-schema to disable.) + --session-track-state-change + Track changes to the 'session state'.

ditto

+ --session-track-system-variables=name + Track changes in registered system variables. + --session-track-transaction-info=name + Track changes to the transaction attributes. OFF to + disable; STATE to track just transaction state (Is there + an active transaction? Does it have any data? etc.); + CHARACTERISTICS to track transaction state and report all + statements needed to start a transaction with the same + characteristics (isolation level, read only/read write, + snapshot - but not any work done / data modified within + the transaction). --show-slave-auth-info Show user and password in SHOW SLAVE HOSTS on this master. @@ -1387,6 +1403,10 @@ safe-user-create FALSE secure-auth TRUE secure-file-priv (No default value) server-id 0 +session-track-schema TRUE +session-track-state-change FALSE +session-track-system-variables autocommit,character_set_client,character_set_connection,character_set_results,time_zone

this is what, a default value? why any session tracking is enabled by default?

+session-track-transaction-info OFF show-slave-auth-info FALSE silent-startup FALSE skip-grant-tables TRUE diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result index 6dca520..b739c67 100644 --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result @@ -3313,6 +3313,34 @@ NUMERIC_BLOCK_SIZE 1 ENUM_VALUE_LIST NULL READ_ONLY NO COMMAND_LINE_ARGUMENT REQUIRED +VARIABLE_NAME SESSION_TRACK_SCHEMA

these variables don't do anything in embedded. do we want to have them here at all?

+SESSION_VALUE ON +GLOBAL_VALUE ON +GLOBAL_VALUE_ORIGIN COMPILE-TIME +DEFAULT_VALUE ON +VARIABLE_SCOPE SESSION +VARIABLE_TYPE BOOLEAN +VARIABLE_COMMENT Track changes to the 'default schema'. +NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST OFF,ON +READ_ONLY NO +COMMAND_LINE_ARGUMENT OPTIONAL +VARIABLE_NAME SESSION_TRACK_STATE_CHANGE +SESSION_VALUE OFF +GLOBAL_VALUE OFF +GLOBAL_VALUE_ORIGIN COMPILE-TIME +DEFAULT_VALUE OFF +VARIABLE_SCOPE SESSION +VARIABLE_TYPE BOOLEAN +VARIABLE_COMMENT Track changes to the 'session state'. +NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST OFF,ON +READ_ONLY NO +COMMAND_LINE_ARGUMENT OPTIONAL VARIABLE_NAME SKIP_EXTERNAL_LOCKING SESSION_VALUE NULL GLOBAL_VALUE ON diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result index 1620579..89999a3 100644 --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result @@ -3775,6 +3775,48 @@ NUMERIC_BLOCK_SIZE 1 ENUM_VALUE_LIST NULL READ_ONLY NO COMMAND_LINE_ARGUMENT REQUIRED +VARIABLE_NAME SESSION_TRACK_SCHEMA +SESSION_VALUE ON +GLOBAL_VALUE ON +GLOBAL_VALUE_ORIGIN COMPILE-TIME +DEFAULT_VALUE ON +VARIABLE_SCOPE SESSION +VARIABLE_TYPE BOOLEAN +VARIABLE_COMMENT Track changes to the 'default schema'. +NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST OFF,ON +READ_ONLY NO +COMMAND_LINE_ARGUMENT OPTIONAL +VARIABLE_NAME SESSION_TRACK_STATE_CHANGE +SESSION_VALUE OFF +GLOBAL_VALUE OFF +GLOBAL_VALUE_ORIGIN COMPILE-TIME +DEFAULT_VALUE OFF +VARIABLE_SCOPE SESSION +VARIABLE_TYPE BOOLEAN +VARIABLE_COMMENT Track changes to the 'session state'. +NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST OFF,ON +READ_ONLY NO +COMMAND_LINE_ARGUMENT OPTIONAL +VARIABLE_NAME SESSION_TRACK_SYSTEM_VARIABLES +SESSION_VALUE autocommit,character_set_client,character_set_connection,character_set_results,time_zone +GLOBAL_VALUE autocommit,character_set_client,character_set_connection,character_set_results,time_zone +GLOBAL_VALUE_ORIGIN COMPILE-TIME +DEFAULT_VALUE autocommit,character_set_client,character_set_connection,character_set_results,time_zone +VARIABLE_SCOPE SESSION +VARIABLE_TYPE VARCHAR +VARIABLE_COMMENT Track changes in registered system variables. +NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST NULL +READ_ONLY NO +COMMAND_LINE_ARGUMENT REQUIRED

where's SESSION_TRACK_TRANSACTION_INFO?

VARIABLE_NAME SKIP_EXTERNAL_LOCKING SESSION_VALUE NULL GLOBAL_VALUE ON diff --git a/mysql-test/suite/sys_vars/t/session_track_system_variables_basic.test b/mysql-test/suite/sys_vars/t/session_track_system_variables_basic.test new file mode 100644 index 0000000..bbb32bb --- /dev/null +++ b/mysql-test/suite/sys_vars/t/session_track_system_variables_basic.test @@ -0,0 +1,133 @@ +--source include/not_embedded.inc + +--echo # +--echo # Variable name : session_track_system_variables +--echo # Scope : Global & Session +--echo # + +--echo # Global - default +SELECT @@global.session_track_system_variables; +--echo # Session - default +SELECT @@session.session_track_system_variables; +--echo + +--echo # via INFORMATION_SCHEMA.GLOBAL_VARIABLES +--disable_warnings +SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES WHERE VARIABLE_NAME LIKE 'session_track%' ORDER BY VARIABLE_NAME; +--enable_warnings

why do you disable warnings here?

+ +--echo # via INFORMATION_SCHEMA.SESSION_VARIABLES +--disable_warnings +SELECT * FROM INFORMATION_SCHEMA.SESSION_VARIABLES WHERE VARIABLE_NAME LIKE 'session_track%' ORDER BY VARIABLE_NAME; +--enable_warnings + +# Save the global value to be used to restore the original value. +SET @global_saved_tmp = @@global.session_track_system_variables; +--echo + +--echo # Altering global variable's value +SET @@global.session_track_system_variables='autocommit'; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Altering session variable's value +SET @@session.session_track_system_variables='autocommit'; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Variables' values in a new session. +connect (con1,"127.0.0.1",root,,test,$MASTER_MYPORT,); + +--echo # Global - expect "autocommit" +SELECT @@global.session_track_system_variables; +--echo +--echo # Session - expect "autocommit" +SELECT @@session.session_track_system_variables; +--echo + +--echo # Switching to the default connection. +connection default; + +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Test if DEFAULT is working as expected. +SET @@global.session_track_system_variables = DEFAULT; +SET @@session.session_track_system_variables = DEFAULT; +--echo + +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Variables' values in a new session (con2). +connect (con2,"127.0.0.1",root,,test,$MASTER_MYPORT,); + +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Altering session should not affect global. +SET @@session.session_track_system_variables = 'sql_mode'; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Variables' values in a new session (con3). +connect (con3,"127.0.0.1",root,,test,$MASTER_MYPORT,); + +--echo # Altering global should not affect session. +SET @@global.session_track_system_variables = 'sql_mode'; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # Switching to the default connection. +connection default; + +--echo # Testing NULL +SET @@global.session_track_system_variables = NULL; +SET @@session.session_track_system_variables = NULL; + +--echo # Global - expect "" instead of NULL +SELECT @@global.session_track_system_variables; +--echo # Session - expect "" instead of NULL +SELECT @@session.session_track_system_variables; + +--echo # testing with duplicate entries. +# Lets first set it to some valid value. +SET @@global.session_track_system_variables= "time_zone"; +SET @@session.session_track_system_variables= "time_zone"; +# Now set with duplicate entries (must pass) +SET @@global.session_track_system_variables= "sql_mode,sql_mode"; +SET @@session.session_track_system_variables= "sql_mode,sql_mode"; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # testing ordering +SET @@global.session_track_system_variables= "time_zone,sql_mode"; +SET @@session.session_track_system_variables= "time_zone,sql_mode"; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + +--echo # special values +SET @@global.session_track_system_variables= "*"; +SET @@session.session_track_system_variables= "*"; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +SET @@global.session_track_system_variables= ""; +SET @@session.session_track_system_variables= ""; +SELECT @@global.session_track_system_variables; +SELECT @@session.session_track_system_variables; +--echo + + +--echo # Restoring the original values. +SET @@global.session_track_system_variables = @global_saved_tmp; + +--echo # End of tests. diff --git a/sql/sql_string.h b/sql/sql_string.h index 51a11c7..feab807 100644 --- a/sql/sql_string.h +++ b/sql/sql_string.h @@ -559,6 +566,17 @@ class String return Ptr+ old_length; /* Area to use */ }

+ inline bool prep_alloc(uint32 arg_length, uint32 step_alloc)

why couldn't you use String::reserve(arg_length, step_alloc) ?

+ { + uint32 new_length= arg_length + str_length; + if (new_length > Alloced_length) + { + if (realloc(new_length + step_alloc)) + return true; + } + return false; + } + inline bool append(const char *s, uint32 arg_length, uint32 step_alloc) { uint32 new_length= arg_length + str_length; @@ -623,6 +641,8 @@ class String { return !sortcmp(this, other, cs); } + void q_net_store_length(ulonglong length); + void q_net_store_data(const uchar *from, size_t length);

why didn't you define these two inline?

};

diff --git a/sql/sql_string.cc b/sql/sql_string.cc index 40339d5..2690310 100644 --- a/sql/sql_string.cc +++ b/sql/sql_string.cc @@ -1157,3 +1158,16 @@ uint convert_to_printable(char *to, size_t to_len, *t= '\0'; return t - to; } + +void String::q_net_store_length(ulonglong length) +{

May be, add DBUG_ASSERT that the buffer is big enough?

+ char *pos= (char *) net_store_length((uchar *)(Ptr + str_length), length); + str_length= pos - Ptr; +} + +void String::q_net_store_data(const uchar *from, size_t length) +{

and here too

+ q_net_store_length(length); + bool res= append((const char *)from, length);

should be q_append here

+ DBUG_ASSERT(!res); +} diff --git a/sql/mysqld.h b/sql/mysqld.h index e538cbd..4173752 100644 --- a/sql/mysqld.h +++ b/sql/mysqld.h @@ -135,6 +135,7 @@ extern my_bool lower_case_file_system; extern my_bool opt_enable_named_pipe, opt_sync_frm, opt_allow_suspicious_udfs; extern my_bool opt_secure_auth; extern const char *current_dbug_option; +extern const char *current_session_track_system_variables;

this 'current_session_track_system_variables' string is not mentioned anywhere else in your patch

extern char* opt_secure_file_priv; extern char* opt_secure_backup_file_priv; extern size_t opt_secure_backup_file_priv_len; diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 845d114..e3aba86 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -691,6 +691,14 @@ THD *next_global_thread(THD *thd) }

struct system_variables global_system_variables; +/** + Following is just for options parsing, used with a difference against + global_system_variables. + + TODO: something should be done to get rid of following variables +*/ +const char *current_dbug_option=""; +

why did you move it?

struct system_variables max_system_variables; struct system_status_var global_status_var;

@@ -5313,6 +5320,17 @@ static int init_server_components() } plugins_are_initialized= TRUE; /* Don't separate from init function */

+ { + Session_tracker session_track_system_variables_check; + if (session_track_system_variables_check. + server_boot_verify(system_charset_info))

make server_boot_verify a class method and invoke it here without creating a useless instance of Session_tracker

+ { + sql_print_error("The variable session_track_system_variables has " + "invalid values."); + unireg_abort(1); + } + } + /* we do want to exit if there are any other unknown options */ if (remaining_argc > 1) { diff --git a/sql/protocol.cc b/sql/protocol.cc index 6469581..73a704a 100644 --- a/sql/protocol.cc +++ b/sql/protocol.cc @@ -197,7 +198,8 @@ bool net_send_error(THD *thd, uint sql_errno, const char *err, @param affected_rows Number of rows changed by statement @param id Auto_increment id for first row (if used) @param message Message to send to the client (Used by mysql_status) - + @param is_eof this called inted of old EOF packet

"instead"

+ @return @retval FALSE The message was successfully sent @retval TRUE An error occurred and the messages wasn't sent properly @@ -209,10 +211,18 @@ bool net_send_ok(THD *thd, uint server_status, uint statement_warn_count, ulonglong affected_rows, ulonglong id, const char *message, + bool is_eof, bool skip_flush) { NET *net= &thd->net; - uchar buff[MYSQL_ERRMSG_SIZE+10],*pos; + StringBuffer<MYSQL_ERRMSG_SIZE + 10> store; + + /* + To be used to manage the data storage in case session state change + information is present. + */

confusing comment, better remove it completely

+ bool state_changed= false; + bool error= FALSE; DBUG_ENTER("net_send_ok");

@@ -222,38 +232,81 @@ net_send_ok(THD *thd, DBUG_RETURN(FALSE); }

- buff[0]=0; // No fields - pos=net_store_length(buff+1,affected_rows); - pos=net_store_length(pos, id); + /* + OK send instead of EOF still require 0xFE header, but OK packet content. + */ + if (is_eof) + { + DBUG_ASSERT(thd->client_capabilities & CLIENT_DEPRECATE_EOF); + store.q_append((char)254); + } + else + store.q_append('\0'); + + /* affected rows */ + store.q_net_store_length(affected_rows); + + /* last insert id */ + store.q_net_store_length(id); + if (thd->client_capabilities & CLIENT_PROTOCOL_41) { DBUG_PRINT("info", ("affected_rows: %lu id: %lu status: %u warning_count: %u", (ulong) affected_rows, (ulong) id, (uint) (server_status & 0xffff), (uint) statement_warn_count)); - int2store(pos, server_status); - pos+=2; + store.q_append2b(server_status);

/* We can only return up to 65535 warnings in two bytes */ uint tmp= MY_MIN(statement_warn_count, 65535); - int2store(pos, tmp); - pos+= 2; + store.q_append2b(tmp); } else if (net->return_status) // For 4.0 protocol { - int2store(pos, server_status); - pos+=2; + store.q_append2b(server_status); } thd->get_stmt_da()->set_overwrite_status(true);

- if (message && message[0]) - pos= net_store_data(pos, (uchar*) message, strlen(message)); - error= my_net_write(net, buff, (size_t) (pos-buff)); - if (!error && !skip_flush) + if ((thd->client_capabilities & CLIENT_SESSION_TRACK)) + { + if (server_status & SERVER_SESSION_STATE_CHANGED) + state_changed= true; + /* the info field */ + if (state_changed || (message && message[0])) + { + store.q_net_store_data((uchar*) message, message ? strlen(message) : 0);

use safe_strlen(message) instead

+ } + + /* session state change information */ + if (unlikely(state_changed)) + { + store.set_charset(thd->variables.collation_database); + + thd->session_tracker.store(thd, &store); + } + } + else if (message && message[0]) + { + /* the info field, if there is a message to store */ + DBUG_ASSERT(strlen(message) <= MYSQL_ERRMSG_SIZE); + store.q_net_store_data((uchar*) message, strlen(message)); + } +

I think, better to write it as state_changed= thd->client_capabilities & CLIENT_SESSION_TRACK && server_status & SERVER_SESSION_STATE_CHANGED; if (state_changed || (message && message[0])) { DBUG_ASSERT(safe_strlen(message) <= MYSQL_ERRMSG_SIZE); store.q_net_store_data((uchar*) safe_str(message), safe_strlen(message)); } if (unlikely(state_changed)) { store.set_charset(thd->variables.collation_database); thd->session_tracker.store(thd, &store); } that is without nested if() and without duplicating message writing lines

+ if (store.length() > MAX_PACKET_LENGTH) + { + net->error= 1; + net->last_errno= ER_NET_OK_PACKET_TOO_LARGE; + my_error(ER_NET_OK_PACKET_TOO_LARGE, MYF(0)); + DBUG_PRINT("info", ("OK packet too large")); + DBUG_RETURN(1);

1. That's kinda unexpected. ER_NET_OK_PACKET_TOO_LARGE when sending an OK packet? 2. does it work at all? I mean, my_error() from inside net_send_ok() - please verify that it's supported.

+ } + error= my_net_write(net, (const unsigned char*)store.ptr(), store.length()); + if (!error && (!skip_flush || is_eof))

Hmm, so you're still flushing for the EOF packet? Why?

error= net_flush(net);

+ thd->server_status&= ~SERVER_SESSION_STATE_CHANGED;

thd->get_stmt_da()->set_overwrite_status(false); DBUG_PRINT("info", ("OK sent, so no more error sending allowed")); @@ -858,14 +928,19 @@ bool Protocol::send_result_set_metadata(List<Item> *list, uint flags)

if (flags & SEND_EOF) { - /* - Mark the end of meta-data result set, and store thd->server_status, - to show that there is no cursor. - Send no warning information, as it will be sent at statement end. - */ - if (write_eof_packet(thd, &thd->net, thd->server_status, - thd->get_stmt_da()->current_statement_warn_count())) - DBUG_RETURN(1); + + /* if it is new client do not send EOF packet */ + if (!(thd->client_capabilities & CLIENT_DEPRECATE_EOF)) + { + /* + Mark the end of meta-data result set, and store thd->server_status, + to show that there is no cursor.

what about "to show that there is no cursor"? new clients won't have this information?

+ Send no warning information, as it will be sent at statement end. + */ + if (write_eof_packet(thd, &thd->net, thd->server_status, + thd->get_stmt_da()->current_statement_warn_count())) + DBUG_RETURN(1); + } } DBUG_RETURN(prepare_for_send(list->elements));

diff --git a/sql/session_tracker.h b/sql/session_tracker.h new file mode 100644 index 0000000..5d529b8 --- /dev/null +++ b/sql/session_tracker.h @@ -0,0 +1,299 @@ +#ifndef SESSION_TRACKER_INCLUDED +#define SESSION_TRACKER_INCLUDED + +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. + Copyright (c) 2016, MariaDB + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ + +#include "m_string.h" +#include "thr_lock.h" + +/* forward declarations */ +class THD; +class set_var; +class String; + + +enum enum_session_tracker +{ + SESSION_SYSVARS_TRACKER, /* Session system variables */ + CURRENT_SCHEMA_TRACKER, /* Current schema */ + SESSION_STATE_CHANGE_TRACKER, + SESSION_GTIDS_TRACKER, /* Tracks GTIDs */ + TRANSACTION_INFO_TRACKER /* Transaction state */ +}; + +#define SESSION_TRACKER_END TRANSACTION_INFO_TRACKER + + +/** + State_tracker + + An abstract class that defines the interface for any of the server's + 'session state change tracker'. A tracker, however, is a sub- class of + this class which takes care of tracking the change in value of a part- + icular session state type and thus defines various methods listed in this + interface. The change information is later serialized and transmitted to + the client through protocol's OK packet. + + Tracker system variables :- + A tracker is normally mapped to a system variable. So in order to enable, + disable or modify the sub-entities of a tracker, the user needs to modify + the respective system variable either through SET command or via command + line option. As required in system variable handling, this interface also + includes two functions to help in the verification of the supplied value + (ON_CHECK) and the updation (ON_UPDATE) of the tracker system variable, + namely - check() and update().

Why do you still have check() method here? It isn't used anywhere. (I've checked out your branch and removed all check() methods and it still compiled fine)

+*/ + +class State_tracker +{ +protected: + /** + Is tracking enabled for a particular session state type ? + + @note: It is cache to avoid virtual functions and checking thd

"it is a cache of the corresponding thd->variables.session_track_xxx variable"

+ when we want mark tracker as changed. + */ + bool m_enabled; + + /** Has the session state type changed ? */ + bool m_changed; + +public: + /** Constructor */ + State_tracker() : m_enabled(false), m_changed(false) + {} + + /** Destructor */ + virtual ~State_tracker() + {} + + /** Getters */ + bool is_enabled() const + { return m_enabled; } + + bool is_changed() const + { return m_changed; } + + /** Called in the constructor of THD*/ + virtual bool enable(THD *thd)= 0; + + /** To be invoked when the tracker's system variable is checked (ON_CHECK). */ + virtual bool check(THD *thd, set_var *var)= 0; + + /** To be invoked when the tracker's system variable is updated (ON_UPDATE).*/ + virtual bool update(THD *thd)= 0; + + /** Store changed data into the given buffer. */ + virtual bool store(THD *thd, String *buf)= 0; + + /** Mark the entity as changed. */ + virtual void mark_as_changed(THD *thd, LEX_CSTRING *name)= 0; +}; + +bool sysvartrack_validate_value(THD *thd, const char *str, size_t len); +bool sysvartrack_reprint_value(THD *thd, char *str, size_t len); +bool sysvartrack_update(THD *thd); +size_t sysvartrack_value_len(THD *thd); +bool sysvartrack_value_construct(THD *thd, char *val, size_t len); + + +/** + Session_tracker + + This class holds an object each for all tracker classes and provides + methods necessary for systematic detection and generation of session + state change information. +*/ + +class Session_tracker +{ +private: + State_tracker *m_trackers[SESSION_TRACKER_END + 1]; + + /* The following two functions are private to disable copying. */ + Session_tracker(Session_tracker const &other) + { + DBUG_ASSERT(FALSE); + } + Session_tracker& operator= (Session_tracker const &rhs) + { + DBUG_ASSERT(FALSE); + return *this; + } + +public: + + Session_tracker(); + ~Session_tracker() + { + deinit(); + } + + /* trick to make happy memory accounting system */ + void deinit() + { + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + { + if (m_trackers[i]) + delete m_trackers[i]; + m_trackers[i]= NULL; + } + } + + void enable(THD *thd); + bool server_boot_verify(const CHARSET_INFO *char_set);

no "const CHARSET_INFO", it's MySQL-ism. We have put "const" into the CHARSET_INFO type definiton.

+ + /** Returns the pointer to the tracker object for the specified tracker. */ + inline State_tracker *get_tracker(enum_session_tracker tracker) const + { + return m_trackers[tracker]; + } + + inline void mark_as_changed(THD *thd, enum enum_session_tracker tracker, + LEX_CSTRING *data) + { + if (m_trackers[tracker]->is_enabled()) + m_trackers[tracker]->mark_as_changed(thd, data); + } + + + void store(THD *thd, String *main_buf); +}; + + +/* + Transaction_state_tracker +*/ + +/** + Transaction state (no transaction, transaction active, work attached, etc.) +*/ +enum enum_tx_state { + TX_EMPTY = 0, ///< "none of the below" + TX_EXPLICIT = 1, ///< an explicit transaction is active + TX_IMPLICIT = 2, ///< an implicit transaction is active + TX_READ_TRX = 4, ///< transactional reads were done + TX_READ_UNSAFE = 8, ///< non-transaction reads were done + TX_WRITE_TRX = 16, ///< transactional writes were done + TX_WRITE_UNSAFE = 32, ///< non-transactional writes were done + TX_STMT_UNSAFE = 64, ///< "unsafe" (non-deterministic like UUID()) stmts + TX_RESULT_SET = 128, ///< result-set was sent

s/result-set/result set/

+ TX_WITH_SNAPSHOT= 256, ///< WITH CONSISTENT SNAPSHOT was used + TX_LOCKED_TABLES= 512 ///< LOCK TABLES is active +}; + + +/** + Transaction access mode +*/ +enum enum_tx_read_flags { + TX_READ_INHERIT = 0, ///< not explicitly set, inherit session.tx_read_only + TX_READ_ONLY = 1, ///< START TRANSACTION READ ONLY, or tx_read_only=1 + TX_READ_WRITE = 2, ///< START TRANSACTION READ WRITE, or tx_read_only=0 +}; + + +/** + Transaction isolation level +*/ +enum enum_tx_isol_level { + TX_ISOL_INHERIT = 0, ///< not explicitly set, inherit session.tx_isolation + TX_ISOL_UNCOMMITTED = 1, + TX_ISOL_COMMITTED = 2, + TX_ISOL_REPEATABLE = 3, + TX_ISOL_SERIALIZABLE= 4 +}; + + +/** + Transaction tracking level +*/ +enum enum_session_track_transaction_info { + TX_TRACK_NONE = 0, ///< do not send tracker items on transaction info + TX_TRACK_STATE = 1, ///< track transaction status + TX_TRACK_CHISTICS = 2 ///< track status and characteristics +}; + + +/** + This is a tracker class that enables & manages the tracking of + current transaction info for a particular connection. +*/ + +class Transaction_state_tracker : public State_tracker +{ +public: + /** Constructor */ + Transaction_state_tracker(); + bool enable(THD *thd) + { return update(thd); } + bool check(THD *thd, set_var *var) + { return false; } + bool update(THD *thd); + bool store(THD *thd, String *buf); + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); + + /** Change transaction characteristics */ + void set_read_flags(THD *thd, enum enum_tx_read_flags flags); + void set_isol_level(THD *thd, enum enum_tx_isol_level level); + + /** Change transaction state */ + void clear_trx_state(THD *thd, uint clear); + void add_trx_state(THD *thd, uint add); + void add_trx_state_from_thd(THD *thd); + void end_trx(THD *thd); + + /** Helper function: turn table info into table access flag */ + enum_tx_state calc_trx_state(THD *thd, thr_lock_type l, bool has_trx); + +private: + enum enum_tx_changed { + TX_CHG_NONE = 0, ///< no changes from previous stmt + TX_CHG_STATE = 1, ///< state has changed from previous stmt + TX_CHG_CHISTICS = 2 ///< characteristics have changed from previous stmt + };

Huh? I've just seen an identical enum just above this class definition?

+ + /** any trackable changes caused by this statement? */ + uint tx_changed; + + /** transaction state */ + uint tx_curr_state, tx_reported_state; + + /** r/w or r/o set? session default? */ + enum enum_tx_read_flags tx_read_flags; + + /** isolation level */ + enum enum_tx_isol_level tx_isol_level; + + void reset(); + + inline void update_change_flags(THD *thd) + { + tx_changed &= ~TX_CHG_STATE; + tx_changed |= (tx_curr_state != tx_reported_state) ? TX_CHG_STATE : 0; + if (tx_changed != TX_CHG_NONE) + mark_as_changed(thd, NULL); + } +}; + +#define TRANSACT_TRACKER(X) \ + do { if (thd->variables.session_track_transaction_info > TX_TRACK_NONE) \ + {((Transaction_state_tracker *) \ + thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER)) \ + ->X; } } while(0) + +#endif /* SESSION_TRACKER_INCLUDED */ diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc new file mode 100644 index 0000000..ae7be5f --- /dev/null +++ b/sql/session_tracker.cc @@ -0,0 +1,1631 @@ +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. + Copyright (c) 2016, MariaDB + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ + + +#include "sql_plugin.h" +#include "session_tracker.h" + +#include "hash.h" +#include "table.h" +#include "rpl_gtid.h" +#include "sql_class.h" +#include "sql_show.h" +#include "sql_plugin.h" +#include "set_var.h" + +class Not_implemented_tracker : public State_tracker +{ +public: + bool enable(THD *thd) + { return false; } + bool check(THD *, set_var *) + { return false; } + bool update(THD *) + { return false; } + bool store(THD *, String *) + { return false; } + void mark_as_changed(THD *, LEX_CSTRING *tracked_item_name) + {} + +}; + +static my_bool name_array_filler(void *ptr, void *data_ptr); +/** + Session_sysvars_tracker + + This is a tracker class that enables & manages the tracking of session + system variables. It internally maintains a hash of user supplied variable + references and a boolean field to store if the variable was changed by the + last statement. +*/ + +class Session_sysvars_tracker : public State_tracker +{ +private: + + struct sysvar_node_st { + sys_var *m_svar; + bool *test_load; + bool m_changed; + };

because you use sysvar_node_st, you need to allocate nodes all the time - many small mallocs. I think this can be avoided, like this: 1. you don't need test_load pointer, if a variable is not loaded, it cannot be changed either, so it's okay to use only m_changed and not look at *test_load. It means that for plugin sysvars you cannot store sys_var* in the hash, you need a new method "void *sys_var::safe_ptr()". Or, may be, "intptr sys_var::tracker_key()". It'll return this for server sysvars and a bookmark for plugin sysvars. 2. as for m_changed, it's just one bit, you can use the lowest bit of the key for this. sys_var::safe_ptr() (or sys_var::tracker_key()) is always aligned, so few lowest bits are free. You can use the second bit to mark all plugin variables (to know whether a pointer is to sysvar or to a bookmark). alternatively, you don't store m_changed in the hash, but keep a list of all modified sysvars. this will also be useful in the store method (and solves the problem or growing hash when '*' is specified) Then you won't need to allocate anything and you can store the key directly in the hash.

+ + class vars_list + { + private: + /** + Registered system variables. (@@session_track_system_variables) + A hash to store the name of all the system variables specified by the + user. + */ + HASH m_registered_sysvars; + /** Size of buffer for string representation */ + size_t buffer_length; + myf m_mem_flag; + /** + If TRUE then we want to check all session variable. + */ + bool track_all; + void init() + { + my_hash_init(&m_registered_sysvars, + &my_charset_bin, + 4, 0, 0, (my_hash_get_key) sysvars_get_key, + my_free, MYF(HASH_UNIQUE | + ((m_mem_flag & MY_THREAD_SPECIFIC) ? + HASH_THREAD_SPECIFIC : 0))); + } + void free_hash() + { + if (my_hash_inited(&m_registered_sysvars)) + { + my_hash_free(&m_registered_sysvars); + } + } + + uchar* search(const sys_var *svar) + { + return (my_hash_search(&m_registered_sysvars, (const uchar *)&svar, + sizeof(sys_var *))); + } + + public: + vars_list() : + buffer_length(0) + { + m_mem_flag= current_thd ? MY_THREAD_SPECIFIC : 0; + init(); + } + + size_t get_buffer_length() + { + DBUG_ASSERT(buffer_length != 0); // asked earlier then should + return buffer_length; + } + ~vars_list() + { + /* free the allocated hash. */ + if (my_hash_inited(&m_registered_sysvars)) + { + my_hash_free(&m_registered_sysvars); + } + } + + uchar* search(sysvar_node_st *node, const sys_var *svar)

may be, rename to insert_or_search ?

+ { + uchar *res; + res= search(svar); + if (!res) + { + if (track_all)

will the hash grow all the time when track_all is true?

+ { + insert(node, svar, m_mem_flag); + return search(svar); + } + } + return res; + } + + uchar* operator[](ulong idx) + { + return my_hash_element(&m_registered_sysvars, idx); + } + bool insert(sysvar_node_st *node, const sys_var *svar, myf mem_flag); + void reset(); + void copy(vars_list* from, THD *thd); + bool parse_var_list(THD *thd, LEX_STRING var_list, bool throw_error, + const CHARSET_INFO *char_set, bool session_created); + bool construct_var_list(char *buf, size_t buf_len); + }; + /** + Two objects of vars_list type are maintained to manage + various operations. + */ + vars_list *orig_list, *tool_list; + +public: + Session_sysvars_tracker() + { + orig_list= new (std::nothrow) vars_list(); + tool_list= new (std::nothrow) vars_list(); + } + + ~Session_sysvars_tracker() + { + if (orig_list) + delete orig_list; + if (tool_list) + delete tool_list; + } + + size_t get_buffer_length() + { + return orig_list->get_buffer_length(); + } + bool construct_var_list(char *buf, size_t buf_len) + { + return orig_list->construct_var_list(buf, buf_len); + } + + /** + Method used to check the validity of string provided + for session_track_system_variables during the server + startup. + */ + static bool server_init_check(THD *thd, const CHARSET_INFO *char_set, + LEX_STRING var_list) + { + vars_list dummy; + bool result; + result= dummy.parse_var_list(thd, var_list, false, char_set, false); + return result; + } + static bool server_init_process(THD *thd, const CHARSET_INFO *char_set, + LEX_STRING var_list) + { + vars_list dummy; + bool result; + result= dummy.parse_var_list(thd, var_list, false, char_set, false); + if (!result) + dummy.construct_var_list(var_list.str, var_list.length + 1); + return result; + } + + void reset(); + bool enable(THD *thd); + bool check(THD *thd, set_var *var); + bool check_str(THD *thd, LEX_STRING val); + bool update(THD *thd); + bool store(THD *thd, String *buf); + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); + /* callback */ + static uchar *sysvars_get_key(const char *entry, size_t *length, + my_bool not_used __attribute__((unused))); + + friend my_bool name_array_filler(void *ptr, void *data_ptr);

why wouldn't you make this one a class method too?

+}; + + + +/** + Current_schema_tracker, + + This is a tracker class that enables & manages the tracking of current + schema for a particular connection. +*/ + +class Current_schema_tracker : public State_tracker +{ +private: + bool schema_track_inited; + void reset(); + +public: + + Current_schema_tracker() + { + schema_track_inited= false; + } + + bool enable(THD *thd) + { return update(thd); } + bool check(THD *thd, set_var *var) + { return false; } + bool update(THD *thd); + bool store(THD *thd, String *buf); + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); +}; + +/* + Session_state_change_tracker + + This is a boolean tracker class that will monitor any change that contributes + to a session state change. + Attributes that contribute to session state change include: + - Successful change to System variables + - User defined variables assignments + - temporary tables created, altered or deleted + - prepared statements added or removed + - change in current database + - change of current role +*/ + +class Session_state_change_tracker : public State_tracker +{ +private: + + void reset(); + +public: + Session_state_change_tracker(); + bool enable(THD *thd) + { return update(thd); }; + bool check(THD *thd, set_var *var) + { return false; } + bool update(THD *thd); + bool store(THD *thd, String *buf); + void mark_as_changed(THD *thd, LEX_CSTRING *tracked_item_name); + bool is_state_changed(THD*); + void ensure_enabled(THD *thd) + {}

this method is never used

+}; + + +/* To be used in expanding the buffer. */ +static const unsigned int EXTRA_ALLOC= 1024; + + +void Session_sysvars_tracker::vars_list::reset() +{ + buffer_length= 0; + track_all= 0; + if (m_registered_sysvars.records) + my_hash_reset(&m_registered_sysvars); +} + +/** + Copy the given list. + + @param from Source vars_list object. + @param thd THD handle to retrive the charset in use. + + @retval true there is something to track + @retval false nothing to track +*/ + +void Session_sysvars_tracker::vars_list::copy(vars_list* from, THD *thd) +{ + reset(); + track_all= from->track_all; + free_hash(); + buffer_length= from->buffer_length; + m_registered_sysvars= from->m_registered_sysvars; + from->init(); +} + +/** + Inserts the variable to be tracked into m_registered_sysvars hash. + + @param node Node to be inserted. + @param svar address of the system variable + + @retval false success + @retval true error +*/ + +bool Session_sysvars_tracker::vars_list::insert(sysvar_node_st *node, + const sys_var *svar, + myf mem_flag) +{ + if (!node) + { + if (!(node= (sysvar_node_st *) my_malloc(sizeof(sysvar_node_st), + MYF(MY_WME | mem_flag)))) + { + reset(); + return true; + } + } + + node->m_svar= (sys_var *)svar; + node->test_load= node->m_svar->test_load; + node->m_changed= false; + if (my_hash_insert(&m_registered_sysvars, (uchar *) node)) + { + my_free(node); + if (!search((sys_var *)svar)) + { + //EOF (error is already reported) + reset(); + return true; + } + } + return false; +} + +/** + Parse the specified system variables list. + + @Note In case of invalid entry a warning is raised per invalid entry. + This is done in order to handle 'potentially' valid system + variables from uninstalled plugins which might get installed in + future. + + + @param thd [IN] The thd handle. + @param var_list [IN] System variable list. + @param throw_error [IN] bool when set to true, returns an error + in case of invalid/duplicate values. + @param char_set [IN] charecter set information used for string + manipulations. + @param session_created [IN] bool variable which says if the parse is + already executed once. The mutex on variables + is not acquired if this variable is false.

why?

+ + @return + true Error + false Success +*/ +bool Session_sysvars_tracker::vars_list::parse_var_list(THD *thd, + LEX_STRING var_list, + bool throw_error, + const CHARSET_INFO *char_set, + bool session_created) +{ + const char separator= ','; + char *token, *lasts= NULL; + size_t rest= var_list.length; + + if (!var_list.str || var_list.length == 0) + { + buffer_length= 1; + return false; + } + + if(!strcmp(var_list.str,(const char *)"*")) + { + track_all= true; + buffer_length= 2; + return false; + } + + buffer_length= var_list.length + 1; + token= var_list.str; + + track_all= false; + /* + If Lock to the plugin mutex is not acquired here itself, it results + in having to acquire it multiple times in find_sys_var_ex for each + token value. Hence the mutex is handled here to avoid a performance + overhead. + */ + if (!thd || session_created) + mysql_mutex_lock(&LOCK_plugin); + for (;;) + { + sys_var *svar; + LEX_STRING var; + + lasts= (char *) memchr(token, separator, rest); + + var.str= token; + if (lasts) + { + var.length= (lasts - token); + rest-= var.length + 1; + } + else + var.length= rest; + + /* Remove leading/trailing whitespace. */ + trim_whitespace(char_set, &var); + + if ((svar= find_sys_var_ex(thd, var.str, var.length, throw_error, true))) + { + if (insert(NULL, svar, m_mem_flag) == TRUE) + goto error; + } + else if (throw_error && session_created && thd) + { + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, + ER_WRONG_VALUE_FOR_VAR, + "%.*s is not a valid system variable and will" + "be ignored.", (int)var.length, token); + } + else + goto error; + + if (lasts) + token= lasts + 1; + else + break; + } + if (!thd || session_created) + mysql_mutex_unlock(&LOCK_plugin); + + return false; + +error: + if (!thd || session_created) + mysql_mutex_unlock(&LOCK_plugin); + return true; +} + +struct name_array_filler_data +{ + LEX_CSTRING **names; + uint idx; + +}; + +/** Collects variable references into array */ +static my_bool name_array_filler(void *ptr, void *data_ptr) +{ + Session_sysvars_tracker::sysvar_node_st *node= + (Session_sysvars_tracker::sysvar_node_st *)ptr; + name_array_filler_data *data= (struct name_array_filler_data *)data_ptr; + if (*node->test_load) + data->names[data->idx++]= &node->m_svar->name; + return FALSE; +} + +/* Sorts variable references array */ +static int name_array_sorter(const void *a, const void *b) +{ + LEX_CSTRING **an= (LEX_CSTRING **)a, **bn=(LEX_CSTRING **)b; + size_t min= MY_MIN((*an)->length, (*bn)->length); + int res= strncmp((*an)->str, (*bn)->str, min); + if (res == 0) + res= ((int)(*bn)->length)- ((int)(*an)->length); + return res; +} + +/** + Construct variable list by internal hash with references +*/ + +bool Session_sysvars_tracker::vars_list::construct_var_list(char *buf, + size_t buf_len) +{ + struct name_array_filler_data data; + size_t left= buf_len; + size_t names_size= m_registered_sysvars.records * sizeof(LEX_CSTRING *); + const char separator= ','; + + if (unlikely(buf_len < 1)) + return true; + + if (unlikely(track_all)) + { + if (buf_len < 2) + return true; + buf[0]= '*'; + buf[1]= '\0'; + return false; + } + + if (m_registered_sysvars.records == 0) + { + buf[0]= '\0'; + return false; + } + + data.names= (LEX_CSTRING**)my_safe_alloca(names_size); + + if (unlikely(!data.names)) + return true; + + data.idx= 0; + + mysql_mutex_lock(&LOCK_plugin); + my_hash_iterate(&m_registered_sysvars, &name_array_filler, &data); + DBUG_ASSERT(data.idx <= m_registered_sysvars.records); + + + if (m_registered_sysvars.records == 0)

you could've done that before my_hash_iterate :)

+ { + mysql_mutex_unlock(&LOCK_plugin); + buf[0]= '\0'; + return false; + } + + my_qsort(data.names, data.idx, sizeof(LEX_CSTRING *), + &name_array_sorter); + + for(uint i= 0; i < data.idx; i++) + { + LEX_CSTRING *nm= data.names[i]; + size_t ln= nm->length + 1; + if (ln > left) + { + mysql_mutex_unlock(&LOCK_plugin); + my_safe_afree(data.names, names_size); + return true; + } + memcpy(buf, nm->str, nm->length); + buf[nm->length]= separator; + buf+= ln; + left-= ln; + } + mysql_mutex_unlock(&LOCK_plugin); + + buf--; buf[0]= '\0'; + my_safe_afree(data.names, names_size); + + return false; +} + +/** + Enable session tracker by parsing global value of tracked variables. + + @param thd [IN] The thd handle. + + @retval true Error + @retval false Success +*/ + +bool Session_sysvars_tracker::enable(THD *thd) +{ + sys_var *svar; + + mysql_mutex_lock(&LOCK_plugin); + svar= find_sys_var_ex(thd, SESSION_TRACK_SYSTEM_VARIABLES_NAME.str, + SESSION_TRACK_SYSTEM_VARIABLES_NAME.length, + false, true); + DBUG_ASSERT(svar); + + set_var tmp(thd, SHOW_OPT_GLOBAL, svar, &null_lex_str, NULL); + svar->session_save_default(thd, &tmp); + + if (tool_list->parse_var_list(thd, tmp.save_result.string_value, + true, thd->charset(), false) == true) + { + mysql_mutex_unlock(&LOCK_plugin); + return true; + } + mysql_mutex_unlock(&LOCK_plugin); + orig_list->copy(tool_list, thd); + m_enabled= true;

This is the only tracker where enable() is different from update(). And the reason for that is that in update() you reuse results of the check, so you only need to parse the string in enable(). But this is wrong. You cannot reuse results of the check(). The example is kind of artificial, but still: SET @@a=1, @@b=2, @@a=3; if the update() method of @@b will fail, @@a final value should be 1. but in your case (@@a is @@session_track_system_variables) it will be 3, because you remember the value of the last check only. So, the correct behavior is (for all sysvars): in check it should only do the check and store the value in the provided set_var::save_result union. in update it should use this value. and if you change session_track_system_variables to behave as other sysvars do, then your enable() will be the same as update(), and you can remove enable() method completely from all trackers.

+ + return false; +} + + +/** + Check system variable name(s). + + @note This function is called from the ON_CHECK() function of the + session_track_system_variables' sys_var class. + + @param thd [IN] The thd handle. + @param var [IN] A pointer to set_var holding the specified list of + system variable names. + + @retval true Error + @retval false Success +*/ + +inline bool Session_sysvars_tracker::check(THD *thd, set_var *var) +{ + return check_str(thd, var->save_result.string_value); +} + +inline bool Session_sysvars_tracker::check_str(THD *thd, LEX_STRING val) +{ + tool_list->reset(); + return tool_list->parse_var_list(thd, val, true, + thd->charset(), true); +} + + +/** + Once the value of the @@session_track_system_variables has been + successfully updated, this function calls + Session_sysvars_tracker::vars_list::copy updating the hash in orig_list + which represents the system variables to be tracked. + + @note This function is called from the ON_UPDATE() function of the + session_track_system_variables' sys_var class. + + @param thd [IN] The thd handle. + + @retval true Error + @retval false Success +*/ + +bool Session_sysvars_tracker::update(THD *thd) +{ + orig_list->copy(tool_list, thd); + return false; +} + + +/** + Store the data for changed system variables in the specified buffer. + Once the data is stored, we reset the flags related to state-change + (see reset()). + + @param thd [IN] The thd handle. + @paran buf [INOUT] Buffer to store the information to. + + @retval true Error + @retval false Success +*/ + +bool Session_sysvars_tracker::store(THD *thd, String *buf) +{ + char val_buf[SHOW_VAR_FUNC_BUFF_SIZE]; + SHOW_VAR show; + const char *value; + sysvar_node_st *node; + const CHARSET_INFO *charset; + size_t val_length, length; + int idx= 0; + + /* As its always system variable. */ + show.type= SHOW_SYS; + + while ((node= (sysvar_node_st *) (*orig_list)[idx]))

Let's be consistent. Either you use my_hash_iterate in construct_var_list() and here. Or you use [] in both functions. Pick the one that you like more and use it.

+ { + if (node->m_changed) + { + mysql_mutex_lock(&LOCK_plugin); + if (!*node->test_load) + { + mysql_mutex_unlock(&LOCK_plugin); + continue; + } + sys_var *svar= node->m_svar; + show.name= svar->name.str; + show.value= (char *) svar; + + value= get_one_variable(thd, &show, OPT_SESSION, SHOW_SYS, NULL, + &charset, val_buf, &val_length);

if you have a sysvar already, you can simply do svar->val_str()

+ mysql_mutex_unlock(&LOCK_plugin); + + length= net_length_size(svar->name.length) + + svar->name.length + + net_length_size(val_length) + + val_length; + + compile_time_assert(SESSION_TRACK_SYSTEM_VARIABLES < 251); + buf->prep_alloc(1 + net_length_size(length) + length, EXTRA_ALLOC); + + /* Session state type (SESSION_TRACK_SYSTEM_VARIABLES) */ + buf->q_net_store_length((ulonglong)SESSION_TRACK_SYSTEM_VARIABLES); + + /* Length of the overall entity. */ + buf->q_net_store_length((ulonglong)length); + + /* System variable's name (length-encoded string). */ + buf->q_net_store_data((const uchar*)svar->name.str, svar->name.length); + + /* System variable's value (length-encoded string). */ + buf->q_net_store_data((const uchar*)value, val_length); + } + ++ idx; + } + + reset(); + + return false; +} + + +/** + Mark the system variable as changed. + + @param [IN] pointer on a variable +*/ + +void Session_sysvars_tracker::mark_as_changed(THD *thd, + LEX_CSTRING *var) +{ + sysvar_node_st *node= NULL; + sys_var *svar= (sys_var *)var; + /* + Check if the specified system variable is being tracked, if so + mark it as changed and also set the class's m_changed flag. + */ + if ((node= (sysvar_node_st *) (orig_list->search(node, svar)))) + { + node->m_changed= true; + m_changed= true; + /* do not cache the statement when there is change in session state */ + thd->lex->safe_to_cache_query= 0; + thd->server_status|= SERVER_SESSION_STATE_CHANGED; + } +} + + +/** + Supply key to a hash. + + @param entry [IN] A single entry. + @param length [OUT] Length of the key. + @param not_used Unused. + + @return Pointer to the key buffer. +*/ + +uchar *Session_sysvars_tracker::sysvars_get_key(const char *entry, + size_t *length, + my_bool not_used __attribute__((unused))) +{ + *length= sizeof(sys_var *); + return (uchar *) &(((sysvar_node_st *) entry)->m_svar); +} + + +/** + Prepare/reset the m_registered_sysvars hash for next statement. +*/ + +void Session_sysvars_tracker::reset() +{ + sysvar_node_st *node; + int idx= 0; + + while ((node= (sysvar_node_st *) (*orig_list)[idx])) + { + node->m_changed= false; + ++ idx; + } + m_changed= false; +} + +static Session_sysvars_tracker* sysvar_tracker(THD *thd) +{ + return (Session_sysvars_tracker*) + thd->session_tracker.get_tracker(SESSION_SYSVARS_TRACKER); +} + +bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) +{ + LEX_STRING tmp= {(char *)str, len}; + if (thd && sysvar_tracker(thd)->is_enabled()) + return sysvar_tracker(thd)->check_str(thd, tmp); + return Session_sysvars_tracker::server_init_check(thd, system_charset_info, + tmp);

this if() won't be needed, if Session_sysvars_tracker::check_str will not store the parsed hash, as I commented earier

+} +bool sysvartrack_reprint_value(THD *thd, char *str, size_t len) +{ + LEX_STRING tmp= {str, len}; + return Session_sysvars_tracker::server_init_process(thd, + system_charset_info, + tmp); +} +bool sysvartrack_update(THD *thd) +{ + return sysvar_tracker(thd)->update(thd); +} +size_t sysvartrack_value_len(THD *thd) +{ + return sysvar_tracker(thd)->get_buffer_length(); +} +bool sysvartrack_value_construct(THD *thd, char *val, size_t len) +{ + return sysvar_tracker(thd)->construct_var_list(val, len); +} + +/////////////////////////////////////////////////////////////////////////////// + +/** + Enable/disable the tracker based on @@session_track_schema's value. + + @param thd [IN] The thd handle. + + @return + false (always) +*/ + +bool Current_schema_tracker::update(THD *thd) +{ + m_enabled= thd->variables.session_track_schema; + return false; +} + + +/** + Store the schema name as length-encoded string in the specified buffer. + + @param thd [IN] The thd handle. + @paran buf [INOUT] Buffer to store the information to. + + @reval false Success + @retval true Error +*/ + +bool Current_schema_tracker::store(THD *thd, String *buf) +{ + ulonglong db_length, length; + + /* + Protocol made (by unknown reasons) redundant: + It saves length of database name and name of database name + + length of saved length of database length. + */ + length= db_length= thd->db_length; + length += net_length_size(length); + + compile_time_assert(SESSION_TRACK_SCHEMA < 251); + compile_time_assert(NAME_LEN < 251); + DBUG_ASSERT(net_length_size(length) < 251);

eh? net_length_size < 251? may be length < 251?

+ if (buf->prep_alloc(1 + 1 + length, EXTRA_ALLOC)) + return true; + + /* Session state type (SESSION_TRACK_SCHEMA) */ + buf->q_net_store_length((ulonglong)SESSION_TRACK_SCHEMA);

you can use buf->q_append((char)SESSION_TRACK_SCHEMA) instead. It's faster and you already know that SESSION_TRACK_SCHEMA will be encoded in one byte. This applies to other similar places too.

+ + /* Length of the overall entity. */ + buf->q_net_store_length(length); + + /* Length and current schema name */ + buf->q_net_store_data((const uchar *)thd->db, thd->db_length); + + reset(); + + return false; +} + + +/** + Mark the tracker as changed. +*/ + +void Current_schema_tracker::mark_as_changed(THD *thd, LEX_CSTRING *) +{ + m_changed= true; + thd->lex->safe_to_cache_query= 0; + thd->server_status|= SERVER_SESSION_STATE_CHANGED;

put this code into the parent State_tracker class then all other trackers won't need to implement it. Only Session_sysvars_tracker will, but still it'll call State_tracker::mark_as_changed(...).

+} + + +/** + Reset the m_changed flag for next statement. + + @return void +*/ + +void Current_schema_tracker::reset() +{ + m_changed= false; +} + + +/////////////////////////////////////////////////////////////////////////////// + + +Transaction_state_tracker::Transaction_state_tracker() +{ + m_enabled = false; + tx_changed = TX_CHG_NONE; + tx_curr_state = + tx_reported_state= TX_EMPTY; + tx_read_flags = TX_READ_INHERIT; + tx_isol_level = TX_ISOL_INHERIT; +} + +/** + Enable/disable the tracker based on @@session_track_transaction_info. + + @param thd [IN] The thd handle. + + @retval true if updating the tracking level failed + @retval false otherwise +*/ + +bool Transaction_state_tracker::update(THD *thd) +{ +#ifdef EMBEDDED_LIBRARY

why you don't do that for other trackers? or, in other words, why do you do it for this tracker?

+ return true; + +#else + if (thd->variables.session_track_transaction_info != TX_TRACK_NONE) + { + /* + If we only just turned reporting on (rather than changing between + state and characteristics reporting), start from a defined state. + */ + if (!m_enabled) + { + tx_curr_state = + tx_reported_state = TX_EMPTY; + tx_changed |= TX_CHG_STATE; + m_enabled= true; + } + if (thd->variables.session_track_transaction_info == TX_TRACK_CHISTICS) + tx_changed |= TX_CHG_CHISTICS; + mark_as_changed(thd, NULL); + } + else + m_enabled= false; + + return false; +#endif +} + + +/** + Store the transaction state (and, optionally, characteristics) + as length-encoded string in the specified buffer. Once the data + is stored, we reset the flags related to state-change (see reset()). + + + @param thd [IN] The thd handle. + @paran buf [INOUT] Buffer to store the information to. + + @retval false Success + @retval true Error +*/ + +static LEX_CSTRING isol[]= { + { STRING_WITH_LEN("READ UNCOMMITTED") }, + { STRING_WITH_LEN("READ COMMITTED") }, + { STRING_WITH_LEN("REPEATABLE READ") }, + { STRING_WITH_LEN("SERIALIZABLE") } +}; + +bool Transaction_state_tracker::store(THD *thd, String *buf) +{ + /* STATE */ + if (tx_changed & TX_CHG_STATE) + { + uchar *to= (uchar *) buf->prep_append(11, EXTRA_ALLOC); + + to= net_store_length((uchar *) to, + (ulonglong) SESSION_TRACK_TRANSACTION_STATE); + + to= net_store_length((uchar *) to, (ulonglong) 9); + to= net_store_length((uchar *) to, (ulonglong) 8); + + *(to++)= (tx_curr_state & TX_EXPLICIT) ? 'T' : + ((tx_curr_state & TX_IMPLICIT) ? 'I' : '_'); + *(to++)= (tx_curr_state & TX_READ_UNSAFE) ? 'r' : '_'; + *(to++)= ((tx_curr_state & TX_READ_TRX) || + (tx_curr_state & TX_WITH_SNAPSHOT)) ? 'R' : '_'; + *(to++)= (tx_curr_state & TX_WRITE_UNSAFE) ? 'w' : '_'; + *(to++)= (tx_curr_state & TX_WRITE_TRX) ? 'W' : '_'; + *(to++)= (tx_curr_state & TX_STMT_UNSAFE) ? 's' : '_'; + *(to++)= (tx_curr_state & TX_RESULT_SET) ? 'S' : '_'; + *(to++)= (tx_curr_state & TX_LOCKED_TABLES) ? 'L' : '_';

This doesn't make much sense, but I suppose that's what MySQL does :( btw, why don't you use String methods here? q_net_store_length and q_append? That'd be consistent with the rest of the code.

+ } + + /* CHARACTERISTICS -- How to restart the transaction */ + + if ((thd->variables.session_track_transaction_info == TX_TRACK_CHISTICS) && + (tx_changed & TX_CHG_CHISTICS)) + { + bool is_xa= (thd->transaction.xid_state.xa_state != XA_NOTR); + size_t start; + + /* 2 length by 1 byte and code */ + buf->prep_alloc(1 + 1 + 1, EXTRA_ALLOC); + + /* Session state type (SESSION_TRACK_TRANSACTION_CHARACTERISTICS) */ + buf->q_net_store_length((ulonglong) + SESSION_TRACK_TRANSACTION_CHARACTERISTICS); + compile_time_assert(SESSION_TRACK_TRANSACTION_CHARACTERISTICS < 251); + + /* Whole length: Track result will fit in 251 byte (in worst case 110) */ + buf->append('\0'); + + /* String length: Track result will fit in 251 byte (in worst case 110) */ + buf->append('\0');

remove these two comments, use q_append, add a comment, like /* placeholders for lengths. will be filled in at the end */

+ + start= buf->length(); + + { + /* + We have four basic replay scenarios: + + a) SET TRANSACTION was used, but before an actual transaction + was started, the load balancer moves the connection elsewhere. + In that case, the same one-shots should be set up in the + target session. (read-only/read-write; isolation-level) + + b) The initial transaction has begun; the relevant characteristics + are the session defaults, possibly overridden by previous + SET TRANSACTION statements, possibly overridden or extended + by options passed to the START TRANSACTION statement. + If the load balancer wishes to move this transaction, + it needs to be replayed with the correct characteristics. + (read-only/read-write from SET or START; + isolation-level from SET only, snapshot from START only) + + c) A subsequent transaction started with START TRANSACTION + (which is legal syntax in lieu of COMMIT AND CHAIN in MySQL) + may add/modify the current one-shots: + + - It may set up a read-only/read-write one-shot. + This one-shot will override the value used in the previous + transaction (whether that came from the default or a one-shot), + and, like all one-shots currently do, it will carry over into + any subsequent transactions that don't explicitly override them + in turn. This behavior is not guaranteed in the docs and may + change in the future, but the tracker item should correctly + reflect whatever behavior a given version of mysqld implements. + + - It may also set up a WITH CONSISTENT SNAPSHOT one-shot. + This one-shot does not currently carry over into subsequent + transactions (meaning that with "traditional syntax", WITH + CONSISTENT SNAPSHOT can only be requested for the first part + of a transaction chain). Again, the tracker item should reflect + mysqld behavior. + + d) A subsequent transaction started using COMMIT AND CHAIN + (or, for that matter, BEGIN WORK, which is currently + legal and equivalent syntax in MySQL, or START TRANSACTION + sans options) will re-use any one-shots set up so far + (with SET before the first transaction started, and with + all subsequent STARTs), except for WITH CONSISTANT SNAPSHOT, + which will never be chained and only applies when explicitly + given. + + It bears noting that if we switch sessions in a follow-up + transaction, SET TRANSACTION would be illegal in the old + session (as a transaction is active), whereas in the target + session which is being prepared, it should be legal, as no + transaction (chain) should have started yet. + + Therefore, we are free to generate SET TRANSACTION as a replay + statement even for a transaction that isn't the first in an + ongoing chain. Consider + + SET TRANSACTION ISOLATION LEVEL READ UNCOMMITED; + START TRANSACTION READ ONLY, WITH CONSISTENT SNAPSHOT; + # work + COMMIT AND CHAIN; + + If we switch away at this point, the replay in the new session + needs to be + + SET TRANSACTION ISOLATION LEVEL READ UNCOMMITED; + START TRANSACTION READ ONLY; + + When a transaction ends (COMMIT/ROLLBACK sans CHAIN), all + per-transaction characteristics are reset to the session's + defaults. + + This also holds for a transaction ended implicitly! (transaction.cc) + Once again, the aim is to have the tracker item reflect on a + given mysqld's actual behavior. + */ + + /* + "ISOLATION LEVEL" + Only legal in SET TRANSACTION, so will always be replayed as such. + */ + if (tx_isol_level != TX_ISOL_INHERIT) + { + /* + Unfortunately, we can't re-use tx_isolation_names / + tx_isolation_typelib as it hyphenates its items. + */ + buf->append(STRING_WITH_LEN("SET TRANSACTION ISOLATION LEVEL ")); + buf->append(isol[tx_isol_level - 1].str, isol[tx_isol_level - 1].length);

this could be simply buf->append(&isol[tx_isol_level - 1]); or even buf->append(isol + tx_isol_level); if you add a dummy element to isol. but whatever... long version works too.

+ buf->append(STRING_WITH_LEN("; ")); + } + + /* + Start transaction will usually result in TX_EXPLICIT (transaction + started, but no data attached yet), except when WITH CONSISTENT + SNAPSHOT, in which case we may have data pending. + If it's an XA transaction, we don't go through here so we can + first print the trx access mode ("SET TRANSACTION READ ...") + separately before adding XA START (whereas with START TRANSACTION, + we can merge the access mode into the same statement). + */ + if ((tx_curr_state & TX_EXPLICIT) && !is_xa) + { + buf->append(STRING_WITH_LEN("START TRANSACTION")); + + /* + "WITH CONSISTENT SNAPSHOT" + Defaults to no, can only be enabled. + Only appears in START TRANSACTION. + */ + if (tx_curr_state & TX_WITH_SNAPSHOT) + { + buf->append(STRING_WITH_LEN(" WITH CONSISTENT SNAPSHOT")); + if (tx_read_flags != TX_READ_INHERIT) + buf->append(STRING_WITH_LEN(",")); + } + + /* + "READ WRITE / READ ONLY" can be set globally, per-session, + or just for one transaction. + + The latter case can take the form of + START TRANSACTION READ (WRITE|ONLY), or of + SET TRANSACTION READ (ONLY|WRITE). + (Both set thd->read_only for the upcoming transaction; + it will ultimately be re-set to the session default.) + + As the regular session-variable tracker does not monitor the one-shot, + we'll have to do it here. + + If READ is flagged as set explicitly (rather than just inherited + from the session's default), we'll get the actual bool from the THD. + */ + if (tx_read_flags != TX_READ_INHERIT) + { + if (tx_read_flags == TX_READ_ONLY) + buf->append(STRING_WITH_LEN(" READ ONLY")); + else + buf->append(STRING_WITH_LEN(" READ WRITE")); + } + buf->append(STRING_WITH_LEN("; ")); + } + else if (tx_read_flags != TX_READ_INHERIT) + { + /* + "READ ONLY" / "READ WRITE" + We could transform this to SET TRANSACTION even when it occurs + in START TRANSACTION, but for now, we'll resysynthesize the original + command as closely as possible. + */ + buf->append(STRING_WITH_LEN("SET TRANSACTION ")); + if (tx_read_flags == TX_READ_ONLY) + buf->append(STRING_WITH_LEN("READ ONLY; ")); + else + buf->append(STRING_WITH_LEN("READ WRITE; ")); + } + + if ((tx_curr_state & TX_EXPLICIT) && is_xa) + { + XID *xid= &thd->transaction.xid_state.xid; + long glen, blen; + + buf->append(STRING_WITH_LEN("XA START")); + + if ((glen= xid->gtrid_length) > 0) + { + buf->append(STRING_WITH_LEN(" '")); + buf->append(xid->data, glen); + + if ((blen= xid->bqual_length) > 0) + { + buf->append(STRING_WITH_LEN("','")); + buf->append(xid->data + glen, blen); + } + buf->append(STRING_WITH_LEN("'")); + + if (xid->formatID != 1) + { + buf->append(STRING_WITH_LEN(",")); + buf->append_ulonglong(xid->formatID); + } + } + + buf->append(STRING_WITH_LEN("; ")); + } + + // discard trailing space + if (buf->length() > start) + buf->length(buf->length() - 1); + } + + { + ulonglong length= buf->length() - start; + uchar *place= (uchar *)(buf->ptr() + (start - 2)); + DBUG_ASSERT(length < 249); // in fact < 110 + DBUG_ASSERT(start >= 3); + + DBUG_ASSERT((place - 1)[0] == SESSION_TRACK_TRANSACTION_CHARACTERISTICS); + /* Length of the overall entity. */ + place[0]= length + 1; + /* Transaction characteristics (length-encoded string). */ + place[1]= length; + } + } + + reset(); + + return false; +} + + +/** + Mark the tracker as changed. +*/ + +void Transaction_state_tracker::mark_as_changed(THD *thd, + LEX_CSTRING *tracked_item_nam) +{ + m_changed= true; + thd->lex->safe_to_cache_query= 0; + thd->server_status|= SERVER_SESSION_STATE_CHANGED; +} + + +/** + Reset the m_changed flag for next statement. +*/ + +void Transaction_state_tracker::reset() +{ + m_changed= false; + tx_reported_state= tx_curr_state; + tx_changed= TX_CHG_NONE; +} + + +/** + Helper function: turn table info into table access flag. + Accepts table lock type and engine type flag (transactional/ + non-transactional), and returns the corresponding access flag + out of TX_READ_TRX, TX_READ_UNSAFE, TX_WRITE_TRX, TX_WRITE_UNSAFE. + + @param thd [IN] The thd handle + @param set [IN] The table's access/lock type + @param set [IN] Whether the table's engine is transactional + + @return The table access flag +*/ + +enum_tx_state Transaction_state_tracker::calc_trx_state(THD *thd, + thr_lock_type l, + bool has_trx) +{ + enum_tx_state s; + bool read= (l <= TL_READ_NO_INSERT); + + if (read) + s= has_trx ? TX_READ_TRX : TX_READ_UNSAFE; + else + s= has_trx ? TX_WRITE_TRX : TX_WRITE_UNSAFE;

I'd do an array lookup, like return arr[l <= TL_READ_NO_INSERT][has_trx]; but whatever...

+ + return s; +} + + +/** + Register the end of an (implicit or explicit) transaction. + + @param thd [IN] The thd handle +*/ +void Transaction_state_tracker::end_trx(THD *thd) +{ + DBUG_ASSERT(thd->variables.session_track_transaction_info > TX_TRACK_NONE); + + if ((!m_enabled) || (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)) + return; + + if (tx_curr_state != TX_EMPTY) + { + if (tx_curr_state & TX_EXPLICIT) + tx_changed |= TX_CHG_CHISTICS; + tx_curr_state &= TX_LOCKED_TABLES; + } + update_change_flags(thd); +} + + +/** + Clear flags pertaining to the current statement or transaction. + May be called repeatedly within the same execution cycle. + + @param thd [IN] The thd handle. + @param set [IN] The flags to clear +*/ + +void Transaction_state_tracker::clear_trx_state(THD *thd, uint clear) +{ + if ((!m_enabled) || (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)) + return; + + tx_curr_state &= ~clear; + update_change_flags(thd); +} + + +/** + Add flags pertaining to the current statement or transaction. + May be called repeatedly within the same execution cycle, + e.g. to add access info for more tables. + + @param thd [IN] The thd handle. + @param set [IN] The flags to add +*/ + +void Transaction_state_tracker::add_trx_state(THD *thd, uint add) +{ + if ((!m_enabled) || (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)) + return; + + if (add == TX_EXPLICIT) + { + /* Always send characteristic item (if tracked), always replace state. */ + tx_changed |= TX_CHG_CHISTICS; + tx_curr_state = TX_EXPLICIT; + } + + /* + If we're not in an implicit or explicit transaction, but + autocommit==0 and tables are accessed, we flag "implicit transaction." + */ + else if (!(tx_curr_state & (TX_EXPLICIT|TX_IMPLICIT)) && + (thd->variables.option_bits & OPTION_NOT_AUTOCOMMIT) && + (add & + (TX_READ_TRX | TX_READ_UNSAFE | TX_WRITE_TRX | TX_WRITE_UNSAFE))) + tx_curr_state |= TX_IMPLICIT; + + /* + Only flag state when in transaction or LOCK TABLES is added. + */ + if ((tx_curr_state & (TX_EXPLICIT | TX_IMPLICIT)) || + (add & TX_LOCKED_TABLES)) + tx_curr_state |= add; + + update_change_flags(thd); +} + + +/** + Add "unsafe statement" flag if applicable. + + @param thd [IN] The thd handle. + @param set [IN] The flags to add +*/ + +void Transaction_state_tracker::add_trx_state_from_thd(THD *thd) +{ + if (m_enabled) + { + if (thd->lex->is_stmt_unsafe()) + add_trx_state(thd, TX_STMT_UNSAFE); + } +} + + +/** + Set read flags (read only/read write) pertaining to the next + transaction. + + @param thd [IN] The thd handle. + @param set [IN] The flags to set +*/ + +void Transaction_state_tracker::set_read_flags(THD *thd, + enum enum_tx_read_flags flags) +{ + if (m_enabled && (tx_read_flags != flags)) + { + tx_read_flags = flags; + tx_changed |= TX_CHG_CHISTICS; + mark_as_changed(thd, NULL); + } +} + + +/** + Set isolation level pertaining to the next transaction. + + @param thd [IN] The thd handle. + @param set [IN] The isolation level to set +*/ + +void Transaction_state_tracker::set_isol_level(THD *thd, + enum enum_tx_isol_level level) +{ + if (m_enabled && (tx_isol_level != level)) + { + tx_isol_level = level; + tx_changed |= TX_CHG_CHISTICS; + mark_as_changed(thd, NULL); + } +} + + +/////////////////////////////////////////////////////////////////////////////// + +Session_state_change_tracker::Session_state_change_tracker() +{ + m_changed= false; +} + +/** + @Enable/disable the tracker based on @@session_track_state_change value. + + @param thd [IN] The thd handle. + @return false (always) + +**/ + +bool Session_state_change_tracker::update(THD *thd) +{ + m_enabled= thd->variables.session_track_state_change; + return false; +} + +/** + Store the '1' in the specified buffer when state is changed. + + @param thd [IN] The thd handle. + @paran buf [INOUT] Buffer to store the information to. + + @reval false Success + @retval true Error +**/ + +bool Session_state_change_tracker::store(THD *thd, String *buf) +{ + if (buf->prep_alloc(1 + 1 + 1, EXTRA_ALLOC)) + return true; + + compile_time_assert(SESSION_TRACK_STATE_CHANGE < 251); + /* Session state type (SESSION_TRACK_STATE_CHANGE) */ + buf->q_net_store_length((ulonglong)SESSION_TRACK_STATE_CHANGE); + + /* Length of the overall entity (1 byte) */ + buf->q_append('\1'); + + DBUG_ASSERT(is_state_changed(thd)); + buf->q_append('1'); + + reset(); + + return false; +} + +/** + Mark the tracker as changed and associated session + attributes accordingly. +*/ + +void Session_state_change_tracker::mark_as_changed(THD *thd, LEX_CSTRING *) +{ + m_changed= true; + thd->lex->safe_to_cache_query= 0; + thd->server_status|= SERVER_SESSION_STATE_CHANGED; +} + +/** + Reset the m_changed flag for next statement. +*/ + +void Session_state_change_tracker::reset() +{ + m_changed= false; +} + +/** + Find if there is a session state change. +*/ + +bool Session_state_change_tracker::is_state_changed(THD *) +{ + return m_changed; +} + +/////////////////////////////////////////////////////////////////////////////// + +/** + @brief Initialize session tracker objects. +*/ + +Session_tracker::Session_tracker() +{ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + m_trackers[i]= NULL; +} + + +/** + @brief Enables the tracker objects. + + @param thd [IN] The thread handle. + + @return void +*/ + +void Session_tracker::enable(THD *thd) +{ + /* + Originally and correctly this allocation was in the constructor and + deallocation in the destructor, but in this case memory counting + system works incorrectly (for example in INSERT DELAYED thread) + */ + deinit(); + m_trackers[SESSION_SYSVARS_TRACKER]= + new (std::nothrow) Session_sysvars_tracker(); + m_trackers[CURRENT_SCHEMA_TRACKER]= + new (std::nothrow) Current_schema_tracker; + m_trackers[SESSION_STATE_CHANGE_TRACKER]= + new (std::nothrow) Session_state_change_tracker; + m_trackers[SESSION_GTIDS_TRACKER]= + new (std::nothrow) Not_implemented_tracker; + m_trackers[TRANSACTION_INFO_TRACKER]= + new (std::nothrow) Transaction_state_tracker;

why do you need to allocate them dynamically at all? Just put them inside Session_tracker object.

+ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + m_trackers[i]->enable(thd); +} + + +/** + Method called during the server startup to verify the contents + of @@session_track_system_variables. + + @retval false Success + @retval true Failure +*/ + +bool Session_tracker::server_boot_verify(const CHARSET_INFO *char_set) +{ + Session_sysvars_tracker *server_tracker; + bool result; + sys_var *svar= find_sys_var_ex(NULL, SESSION_TRACK_SYSTEM_VARIABLES_NAME.str, + SESSION_TRACK_SYSTEM_VARIABLES_NAME.length, + false, true); + DBUG_ASSERT(svar); + set_var tmp(NULL, SHOW_OPT_GLOBAL, svar, &null_lex_str, NULL); + svar->session_save_default(NULL, &tmp); + server_tracker= new (std::nothrow) Session_sysvars_tracker(); + result= server_tracker->server_init_check(NULL, char_set, + tmp.save_result.string_value); + delete server_tracker;

grrrr. really? svar->session_save_default() and new/delete a tracker? just to verify the value?

+ return result; +} + + +/** + @brief Store all change information in the specified buffer. + + @param thd [IN] The thd handle. + @param buf [OUT] Reference to the string buffer to which the state + change data needs to be written. +*/ + +void Session_tracker::store(THD *thd, String *buf) +{ + size_t start; + + /* + Probably most track result will fit in 251 byte so lets made it at + least efficient. We allocate 1 byte for length and then will move + string if there is more. + */ + buf->append('\0'); + start= buf->length(); + + /* Get total length. */ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + { + if (m_trackers[i]->is_changed() && + m_trackers[i]->store(thd, buf)) + { + buf->length(start); // it is safer to have 0-length block in case of error + return; + } + } + + size_t length= buf->length() - start; + uchar *data= (uchar *)(buf->ptr() + start); + uint size; + + if ((size= net_length_size(length)) != 1) + { + if (buf->prep_alloc(size - 1, EXTRA_ALLOC)) + { + buf->length(start); // it is safer to have 0-length block in case of error + return; + } + memmove(data + (size - 1), data, length); + } + + net_store_length(data - 1, length); +} diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 7dfc622..c9e83ae 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7168,6 +7167,10 @@ skip-to-error-number 3000 ER_MYSQL_57_TEST eng "5.7 test"

+ER_NET_OK_PACKET_TOO_LARGE 08S01 + eng "OK packet too large" + ukr "Пакет OK надто великий"

as I've said above, this is a rather strange error.

+ # MariaDB extra error numbers starts from 4000 skip-to-error-number 4000

diff --git a/sql/lock.cc b/sql/lock.cc index 2e44786..e2d2da0 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -89,6 +89,7 @@ extern HASH open_cache;

static int lock_external(THD *thd, TABLE **table,uint count); static int unlock_external(THD *thd, TABLE **table,uint count); +static void track_table_access(THD *thd, TABLE **tables, size_t count);

this forward declaration is not needed

/* Map the return value of thr_lock to an error from errmsg.txt */ static int thr_lock_errno_to_mysql[]= @@ -244,6 +245,39 @@ void reset_lock_data(MYSQL_LOCK *sql_lock, bool unlock)

/** + Scan array of tables for access types; update transaction tracker + accordingly. + + @param thd The current thread. + @param tables An array of pointers to the tables to lock. + @param count The number of tables to lock. +*/ + +static void track_table_access(THD *thd, TABLE **tables, size_t count) +{ + if (thd->variables.session_track_transaction_info > TX_TRACK_NONE) + { + Transaction_state_tracker *tst= (Transaction_state_tracker *) + thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER); + enum enum_tx_state s; + + while (count--) + { + TABLE *t= tables[count]; + + if (t) + { + s= tst->calc_trx_state(thd, t->reginfo.lock_type, + t->file->has_transactions());

don't do calc_trx_state, turn it into a calc_and_add_trx_state. or, may be, tst->add_trx_state(thd, t->reginfo.lock_type, t->file->has_transactions()) because in all cases where calc_trx_state is used, the caller calls add_trx_state immediately after that.

+ tst->add_trx_state(thd, s); + } + } + } +} + + + +/** Lock tables.

@param thd The current thread. diff --git a/sql/set_var.cc b/sql/set_var.cc index 5392a00..5f2bc93 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -204,8 +209,29 @@ bool sys_var::update(THD *thd, set_var *var) (on_update && on_update(this, thd, OPT_GLOBAL)); } else - return session_update(thd, var) || + { + bool ret= session_update(thd, var) || (on_update && on_update(this, thd, OPT_SESSION)); + + /* + Make sure we don't session-track variables that are not actually + part of the session. tx_isolation and and tx_read_only for example + exist as GLOBAL, SESSION, and one-shot ("for next transaction only"). + */ + if ((var->type == OPT_SESSION) && (!ret))

first, s/var->type/type/ second, when type is not OPT_SESSION? only for tx_isolation and tx_read_only? btw, why that many parentheses?

+ { + thd->session_tracker.mark_as_changed(thd, SESSION_SYSVARS_TRACKER, + (LEX_CSTRING*)var->var); + /* + Here MySQL sends variable name to avoid reporting change of + the tracker itself, but we decided that it is not needed + */ + thd->session_tracker.mark_as_changed(thd, SESSION_STATE_CHANGE_TRACKER, + NULL); + } + + return ret; + } }

uchar *sys_var::session_value_ptr(THD *thd, const LEX_STRING *base) diff --git a/sql/sp_head.cc b/sql/sp_head.cc index d58b51a..396f308 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -2975,6 +2977,16 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp,

reinit_stmt_before_use(thd, m_lex);

+#ifndef EMBEDDED_LIBRARY + /* + if there was instruction which changed tracking state before, result + can go with this command OK packet, so better do not cache the result.

sorry, I cannot parse that :(

+ */ + if ((thd->client_capabilities & CLIENT_SESSION_TRACK) && + (thd->server_status & SERVER_SESSION_STATE_CHANGED)) + thd->lex->safe_to_cache_query= 0; +#endif + if (open_tables) res= instr->exec_open_and_lock_tables(thd, m_lex->query_tables);

diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 08a9647..924e9d4 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -5013,6 +5018,20 @@ static bool check_lock_and_start_stmt(THD *thd, table_list->table->file->print_error(error, MYF(0)); DBUG_RETURN(1); } + + /* + Record in transaction state tracking + */ + if (thd->variables.session_track_transaction_info > TX_TRACK_NONE) + { + Transaction_state_tracker *tst= (Transaction_state_tracker *) + thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER); + enum enum_tx_state s= + tst->calc_trx_state(thd, lock_type, + table_list->table->file->has_transactions()); + tst->add_trx_state(thd, s);

after you get rid of a separatre calc_trx_state call, you'll be able to use TRANSACT_TRACKER here too

+ } + DBUG_RETURN(0); }

diff --git a/sql/sql_cache.cc b/sql/sql_cache.cc index 42b80d9..b0dacca 100644 --- a/sql/sql_cache.cc +++ b/sql/sql_cache.cc @@ -1381,6 +1381,19 @@ void Query_cache::store_query(THD *thd, TABLE_LIST *tables_used) DBUG_VOID_RETURN; }

+ /* + Do not store queries while tracking transaction state. + The tracker already flags queries that actually have + transaction tracker items, but this will make behavior + more straight forward.

why?

+ */ + if (thd->variables.session_track_transaction_info != TX_TRACK_NONE) + { + DBUG_PRINT("qcache", ("Do not work with transaction tracking")); + DBUG_VOID_RETURN; + } + + /* The following assert fails if we haven't called send_result_to_client */ DBUG_ASSERT(thd->base_query.is_alloced() || thd->base_query.ptr() == thd->query()); @@ -1719,6 +1732,18 @@ Query_cache::send_result_to_client(THD *thd, char *org_sql, uint query_length) goto err; }

+ /* + Don't allow serving from Query_cache while tracking transaction + state. This is a safeguard in case an otherwise matching query + was added to the cache before tracking was turned on.

how could that be possible?

+ */ + if (thd->variables.session_track_transaction_info != TX_TRACK_NONE) + { + DBUG_PRINT("qcache", ("Do not work with transaction tracking")); + goto err; + } + + thd->query_cache_is_applicable= 1; sql= org_sql; sql_end= sql + query_length;

diff --git a/sql/sql_db.cc b/sql/sql_db.cc index 2ba67cb..1780a81 100644 --- a/sql/sql_db.cc +++ b/sql/sql_db.cc @@ -1034,7 +1034,10 @@ mysql_rm_db_internal(THD *thd,char *db, bool if_exists, bool silent) it to 0. */ if (thd->db && cmp_db_names(thd->db, db) && !error) + { mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server); + thd->session_tracker.mark_as_changed(thd, CURRENT_SCHEMA_TRACKER, NULL);

another point for simply including all trackers into Session_tracker. this method will be inlined, the check for is_enabled will be inlined too, so in the normal case this will become much cheaper.

+ } my_dirend(dirp); DBUG_RETURN(error); } diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index f540c26..0342a88 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -269,6 +269,7 @@ struct st_bookmark uint name_len; int offset; uint version; + bool loaded;

I'm not commenting on changes in this file, because I hope they will go away completely, see my earlier comment.

char key[1]; };

@@ -322,6 +323,8 @@ static void unlock_variables(THD *thd, struct system_variables *vars); static void cleanup_variables(struct system_variables *vars); static void plugin_vars_free_values(sys_var *vars); static void restore_ptr_backup(uint n, st_ptr_backup *backup); +#define my_intern_plugin_lock(A,B) intern_plugin_lock(A,B) +#define my_intern_plugin_lock_ci(A,B) intern_plugin_lock(A,B)

nope. remove those macros, as I've already wrote in a previous review. I introduced them to track memory allocations (all _ci macros pass "caller info" down the stack, __FILE__ and __LINE__, so that safemalloc would show not the line where malloc was called, but where, say, init_dynamic_array was called). in MariaDB this is not needed, because I've changed safemalloc to remember stack traces. So all _ci macros are long gone now, don't add them back from MySQL code.

static plugin_ref intern_plugin_lock(LEX *lex, plugin_ref plugin); static void intern_plugin_unlock(LEX *lex, plugin_ref plugin); static void reap_plugins(void); diff --git a/sql/sql_show.cc b/sql/sql_show.cc index f41fb39..b6ed6e6 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -3272,109 +3398,21 @@ static bool show_status_array(THD *thd, const char *wild, name_buffer, wild))) && (!cond || cond->val_int())) { - void *value=var->value; - const char *pos, *end; // We assign a lot of const's + const char *pos; // We assign a lot of const's + size_t length;

if (show_type == SHOW_SYS) - { - sys_var *var= (sys_var *) value; - show_type= var->show_type(); mysql_mutex_lock(&LOCK_global_system_variables); - value= var->value_ptr(thd, scope, &null_lex_str); - charset= var->charset(thd); - } + pos= get_one_variable(thd, var, scope, show_type, status_var, + &charset, buff, &length);

this wasn't needed, if you won't use get_one_variable() anywhere else

- pos= end= buff; - /* - note that value may be == buff. All SHOW_xxx code below - should still work in this case - */ - switch (show_type) { - case SHOW_DOUBLE_STATUS: - value= ((char *) status_var + (intptr) value); - /* fall through */ - case SHOW_DOUBLE: - /* 6 is the default precision for '%f' in sprintf() */ - end= buff + my_fcvt(*(double *) value, 6, buff, NULL); - break; - case SHOW_LONG_STATUS: - value= ((char *) status_var + (intptr) value); - /* fall through */ - case SHOW_ULONG: - case SHOW_LONG_NOFLUSH: // the difference lies in refresh_status() - end= int10_to_str(*(long*) value, buff, 10); - break; - case SHOW_LONGLONG_STATUS: - value= ((char *) status_var + (intptr) value); - /* fall through */ - case SHOW_ULONGLONG: - end= longlong10_to_str(*(longlong*) value, buff, 10); - break; - case SHOW_HA_ROWS: - end= longlong10_to_str((longlong) *(ha_rows*) value, buff, 10); - break; - case SHOW_BOOL: - end= strmov(buff, *(bool*) value ? "ON" : "OFF"); - break; - case SHOW_MY_BOOL: - end= strmov(buff, *(my_bool*) value ? "ON" : "OFF"); - break; - case SHOW_UINT: - end= int10_to_str((long) *(uint*) value, buff, 10); - break; - case SHOW_SINT: - end= int10_to_str((long) *(int*) value, buff, -10); - break; - case SHOW_SLONG: - end= int10_to_str(*(long*) value, buff, -10); - break; - case SHOW_SLONGLONG: - end= longlong10_to_str(*(longlong*) value, buff, -10); - break; - case SHOW_HAVE: - { - SHOW_COMP_OPTION tmp= *(SHOW_COMP_OPTION*) value; - pos= show_comp_option_name[(int) tmp]; - end= strend(pos); - break; - } - case SHOW_CHAR: - { - if (!(pos= (char*)value)) - pos= ""; - end= strend(pos); - break; - } - case SHOW_CHAR_PTR: - { - if (!(pos= *(char**) value)) - pos= ""; - - end= strend(pos); - break; - } - case SHOW_LEX_STRING: - { - LEX_STRING *ls=(LEX_STRING*)value; - if (!(pos= ls->str)) - end= pos= ""; - else - end= pos + ls->length; - break; - } - case SHOW_UNDEF: - break; // Return empty string - case SHOW_SYS: // Cannot happen - default: - DBUG_ASSERT(0); - break; - } - table->field[1]->store(pos, (uint32) (end - pos), charset); + table->field[1]->store(pos, (uint32) length, charset); + thd->count_cuted_fields= CHECK_FIELD_IGNORE; table->field[1]->set_notnull(); - - if (var->type == SHOW_SYS) + if (show_type == SHOW_SYS) mysql_mutex_unlock(&LOCK_global_system_variables);

+ if (schema_table_store_record(thd, table)) { res= TRUE; diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 4bf2028..3be2e55 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -5365,3 +5377,72 @@ static Sys_var_ulong Sys_log_tc_size( DEFAULT(my_getpagesize() * 6), BLOCK_SIZE(my_getpagesize())); #endif + +const LEX_CSTRING SESSION_TRACK_SYSTEM_VARIABLES_NAME= + {STRING_WITH_LEN("session_track_system_variables")};

I think you don't need that^^^. You shouldn't search for Sys_var_sesvartrack by name anyway.

+ +static Sys_var_sesvartrack Sys_track_session_sys_vars( + SESSION_TRACK_SYSTEM_VARIABLES_NAME.str, + "Track changes in registered system variables.", + CMD_LINE(REQUIRED_ARG), IN_SYSTEM_CHARSET, + DEFAULT("autocommit,character_set_client,character_set_connection," + "character_set_results,time_zone"), + NO_MUTEX_GUARD); + +static bool update_session_track_schema(sys_var *self, THD *thd, + enum_var_type type) +{ + DBUG_ENTER("update_session_track_schema"); + DBUG_RETURN(thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->update(thd)); +} + +static Sys_var_mybool Sys_session_track_schema( + "session_track_schema", + "Track changes to the 'default schema'.", + SESSION_VAR(session_track_schema), + CMD_LINE(OPT_ARG), DEFAULT(TRUE), + NO_MUTEX_GUARD, NOT_IN_BINLOG, + ON_CHECK(0), + ON_UPDATE(update_session_track_schema)); + + +static bool update_session_track_tx_info(sys_var *self, THD *thd, + enum_var_type type) +{ + DBUG_ENTER("update_session_track_tx_info"); + DBUG_RETURN(thd->session_tracker.get_tracker(TRANSACTION_INFO_TRACKER)->update(thd)); +} + +static const char *session_track_transaction_info_names[]= + { "OFF", "STATE", "CHARACTERISTICS", NullS }; + +static Sys_var_enum Sys_session_track_transaction_info( + "session_track_transaction_info", + "Track changes to the transaction attributes. OFF to disable; " + "STATE to track just transaction state (Is there an active transaction? " + "Does it have any data? etc.); CHARACTERISTICS to track transaction " + "state " + "and report all statements needed to start a transaction with the same " + "characteristics (isolation level, read only/read write, snapshot - " + "but not any work done / data modified within the transaction).", + SESSION_VAR(session_track_transaction_info), + CMD_LINE(REQUIRED_ARG), session_track_transaction_info_names, + DEFAULT(0), NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), + ON_UPDATE(update_session_track_tx_info)); + + +static bool update_session_track_state_change(sys_var *self, THD *thd, + enum_var_type type) +{ + DBUG_ENTER("update_session_track_state_change"); + DBUG_RETURN(thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->update(thd)); +} + +static Sys_var_mybool Sys_session_track_state_change( + "session_track_state_change", + "Track changes to the 'session state'.", + SESSION_VAR(session_track_state_change), + CMD_LINE(OPT_ARG), DEFAULT(FALSE), + NO_MUTEX_GUARD, NOT_IN_BINLOG, + ON_CHECK(0), + ON_UPDATE(update_session_track_state_change)); diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index 2488e80..cf01835 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -535,6 +537,105 @@ class Sys_var_charptr: public sys_var } };

+class Sys_var_charptr: public Sys_var_charptr_base +{ +public: + Sys_var_charptr(const char *name_arg, + const char *comment, int flag_args, ptrdiff_t off, size_t size, + CMD_LINE getopt, + enum charset_enum is_os_charset_arg, + const char *def_val, PolyLock *lock=0, + enum binlog_status_enum binlog_status_arg=VARIABLE_NOT_IN_BINLOG, + on_check_function on_check_func=0, + on_update_function on_update_func=0, + const char *substitute=0) : + Sys_var_charptr_base(name_arg, comment, flag_args, off, size, getopt, + is_os_charset_arg, def_val, lock, binlog_status_arg, + on_check_func, on_update_func, substitute) + { + SYSVAR_ASSERT(scope() == GLOBAL); + SYSVAR_ASSERT(size == sizeof(char *)); + } + + bool session_update(THD *thd, set_var *var) + { + DBUG_ASSERT(FALSE); + return true; + } + void session_save_default(THD *thd, set_var *var) + { DBUG_ASSERT(FALSE); } +}; + +class Sys_var_sesvartrack: public Sys_var_charptr_base +{ +public: + Sys_var_sesvartrack(const char *name_arg, + const char *comment, + CMD_LINE getopt, + enum charset_enum is_os_charset_arg, + const char *def_val, PolyLock *lock) : + Sys_var_charptr_base(name_arg, comment, + SESSION_VAR(session_track_system_variables), + getopt, is_os_charset_arg, def_val, lock, + VARIABLE_NOT_IN_BINLOG, 0, 0, 0) + {} + bool do_check(THD *thd, set_var *var) + { + if (Sys_var_charptr_base::do_check(thd, var) || + sysvartrack_validate_value(thd, var->save_result.string_value.str, + var->save_result.string_value.length)) + return TRUE; + return FALSE; + } + bool global_update(THD *thd, set_var *var) + { + char *new_val= global_update_prepare(thd, var); + if (new_val) + { + if (sysvartrack_reprint_value(thd, new_val, + var->save_result.string_value.length)) + new_val= 0; + } + global_update_finish(new_val); + return (new_val == 0 && var->save_result.string_value.str != 0); + } + bool session_update(THD *thd, set_var *var) + { + return sysvartrack_update(thd); + } + void session_save_default(THD *thd, set_var *var) + { + var->save_result.string_value.str= global_var(char*); + var->save_result.string_value.length= + strlen(var->save_result.string_value.str); + /* parse and feel list with default values */ + if (thd) + { + bool res= + sysvartrack_validate_value(thd, + var->save_result.string_value.str, + var->save_result.string_value.length); + DBUG_ASSERT(res == 0); + } + } + uchar *session_value_ptr(THD *thd, const LEX_STRING *base) + { + DBUG_ASSERT(thd != NULL); + size_t len= sysvartrack_value_len(thd); + char *res= 0; + char *buf= (char *)my_safe_alloca(len); + if (buf && !sysvartrack_value_construct(thd, buf, len)) + { + size_t len= strlen(buf) + 1; + res= (char*) thd->alloc(len + sizeof(char *));

why don't you use thd->alloc up front? why alloca and then memcpy?

+ if (res) + memcpy((*((char**) res)= res + sizeof(char *)), buf, len); + my_safe_afree(buf, len); + } + return (uchar *)res; + } +}; +

class Sys_var_proxy_user: public sys_var {

Regards, Sergei Chief Architect MariaDB and security@mariadb.org