Re: [Maria-developers] 4c51152: MDEV-8931: (server part of) session state tracking

Hi, Sanja! Summary: Lots of comments below, many changes needed. When you copy a big feature from MySQL next time - please, take a more critical look at what you're merging. On Mar 28, OleksandrByelkin wrote:

revision-id: 4c51152d9f43e271e17a2bc266f5887ce092c00f (mariadb-10.1.8-187-g4c51152) parent(s): 69b5c4a4220c8fa98bbca8b3f935b2a3735e19ac committer: Oleksandr Byelkin timestamp: 2016-03-28 19:19:54 +0200 message:

MDEV-8931: (server part of) session state tracking

diff --git a/include/mysql_com.h b/include/mysql_com.h index c13999a..c6608b7 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. +*/ +enum enum_session_state_type +{ + SESSION_TRACK_SYSTEM_VARIABLES, /* Session system variables */

support comes in the next patch, I know

+ SESSION_TRACK_SCHEMA, /* Current schema */

suported, I presume

+ SESSION_TRACK_STATE_CHANGE, /* track session state changes */

suported, I presume

+ SESSION_TRACK_GTIDS,

not suported, I presume

+ SESSION_TRACK_TRANSACTION_CHARACTERISTICS, /* Transaction chistics */ + SESSION_TRACK_TRANSACTION_STATE /* Transaction state */

to be imlemented later, I suppose

+}; + +#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/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result index a35693e..f69b615 100644 --- a/mysql-test/r/mysqld--help.result +++ b/mysql-test/r/mysqld--help.result @@ -903,6 +903,11 @@ 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'. + (Defaults to on; use --skip-session-track-schema to disable.) + --session-track-state-change + Track changes to the 'session state'. --show-slave-auth-info Show user and password in SHOW SLAVE HOSTS on this master. diff --git a/mysql-test/suite/sys_vars/r/session_track_schema_basic.result b/mysql-test/suite/sys_vars/r/session_track_schema_basic.result new file mode 100644 index 0000000..be4b892 --- /dev/null +++ b/mysql-test/suite/sys_vars/r/session_track_schema_basic.result @@ -0,0 +1,116 @@

we don't need these "basic" sysvar tests anymore. The test that checks for them is disabled. sysvar_* tests are now used to show basic sysvar characteristics

+# +# Variable name : session_track_schema +# Scope : Global & Session +# +# Global - default +SELECT @@global.session_track_schema; +@@global.session_track_schema +1 +# Session - default +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 + +# via INFORMATION_SCHEMA.GLOBAL_VARIABLES +SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; +VARIABLE_NAME VARIABLE_VALUE +# via INFORMATION_SCHEMA.SESSION_VARIABLES +SELECT * FROM INFORMATION_SCHEMA.SESSION_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; +VARIABLE_NAME VARIABLE_VALUE +SET @global_saved_tmp = @@global.session_track_schema; + +# Altering global variable's value +SET @@global.session_track_schema = 0; +SELECT @@global.session_track_schema; +@@global.session_track_schema +0 +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 +SET @@global.session_track_schema = TrUe; +SELECT @@global.session_track_schema; +@@global.session_track_schema +1 +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 +SET @@global.session_track_schema = FaLsE; +SELECT @@global.session_track_schema; +@@global.session_track_schema +0 +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 + +# Altering session variable's value +SET @@session.session_track_schema = 0; +SELECT @@global.session_track_schema; +@@global.session_track_schema +0 +SELECT @@session.session_track_schema; +@@session.session_track_schema +0 + +# Variables' values in a new session. +# Global - expect 0 +SELECT @@global.session_track_schema; +@@global.session_track_schema +0 + +# Session - expect 0 +SELECT @@session.session_track_schema; +@@session.session_track_schema +0 + +# Switching to the default connection. +SELECT @@global.session_track_schema; +@@global.session_track_schema +0 +SELECT @@session.session_track_schema; +@@session.session_track_schema +0 + +# Test if DEFAULT is working as expected. +SET @@global.session_track_schema = DEFAULT; +SET @@session.session_track_schema = DEFAULT; + +# Global - expect 1 +SELECT @@global.session_track_schema; +@@global.session_track_schema +1 +# Session - expect 1 +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 + +# Variables' values in a new session (con2). +SELECT @@global.session_track_schema; +@@global.session_track_schema +1 +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 + +# Altering session should not affect global. +SET @@session.session_track_schema = FALSE; +SELECT @@global.session_track_schema; +@@global.session_track_schema +1 +SELECT @@session.session_track_schema; +@@session.session_track_schema +0 + +# Variables' values in a new session (con3). +# Altering global should not affect session. +SET @@global.session_track_schema = OFF; +SELECT @@global.session_track_schema; +@@global.session_track_schema +0 +SELECT @@session.session_track_schema; +@@session.session_track_schema +1 + +# Switching to the default connection. +# Restoring the original values. +SET @@global.session_track_schema = @global_saved_tmp; +# End of tests. diff --git a/mysql-test/suite/sys_vars/t/session_track_schema_basic.test b/mysql-test/suite/sys_vars/t/session_track_schema_basic.test new file mode 100644 index 0000000..220ae35 --- /dev/null +++ b/mysql-test/suite/sys_vars/t/session_track_schema_basic.test @@ -0,0 +1,105 @@

another meaningless basic test

+--source include/not_embedded.inc + +--echo # +--echo # Variable name : session_track_schema +--echo # Scope : Global & Session +--echo # + +--echo # Global - default +SELECT @@global.session_track_schema; +--echo # Session - default +SELECT @@session.session_track_schema; +--echo + +--echo # via INFORMATION_SCHEMA.GLOBAL_VARIABLES +--disable_warnings +SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; +--enable_warnings + +--echo # via INFORMATION_SCHEMA.SESSION_VARIABLES +--disable_warnings +SELECT * FROM INFORMATION_SCHEMA.SESSION_VARIABLES WHERE VARIABLE_NAME LIKE 'track_current%'; +--enable_warnings + +# Save the global value to be used to restore the original value. +SET @global_saved_tmp = @@global.session_track_schema; +--echo + +--echo # Altering global variable's value +SET @@global.session_track_schema = 0; +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; + +SET @@global.session_track_schema = TrUe; +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; + +SET @@global.session_track_schema = FaLsE; +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; +--echo + +--echo # Altering session variable's value +SET @@session.session_track_schema = 0; +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; +--echo + +--echo # Variables' values in a new session. +connect (con1,"127.0.0.1",root,,test,$MASTER_MYPORT,); + +--echo # Global - expect 0 +SELECT @@global.session_track_schema; +--echo +--echo # Session - expect 0 +SELECT @@session.session_track_schema; +--echo + +--echo # Switching to the default connection. +connection default; + +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; +--echo + +--echo # Test if DEFAULT is working as expected. +SET @@global.session_track_schema = DEFAULT; +SET @@session.session_track_schema = DEFAULT; +--echo + +--echo # Global - expect 1 +SELECT @@global.session_track_schema; +--echo # Session - expect 1 +SELECT @@session.session_track_schema; +--echo + +--echo # Variables' values in a new session (con2). +connect (con2,"127.0.0.1",root,,test,$MASTER_MYPORT,); + +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; +--echo + +--echo # Altering session should not affect global. +SET @@session.session_track_schema = FALSE; +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; +--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_schema = OFF; +SELECT @@global.session_track_schema; +SELECT @@session.session_track_schema; +--echo + +--echo # Switching to the default connection. +connection default; + +--echo # Restoring the original values. +SET @@global.session_track_schema = @global_saved_tmp; + +--echo # End of tests. + diff --git a/sql/protocol.cc b/sql/protocol.cc index 9e52870..c1e66d7 100644 --- a/sql/protocol.cc +++ b/sql/protocol.cc @@ -197,6 +197,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 eof_indentifier when true [FE] will be set in OK header + else [00] will be used

eh, I understand that's what MySQL has, and we want to be protocol-compatible, but can you put a meaningful comment here, please? Not like i++; // increment i by 1 and a better name for the parameter would be nice too.

@return @retval FALSE The message was successfully sent @@ -221,9 +233,32 @@ 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); + start= buff; + + /* + Use 0xFE packet header if eof_identifier is true + unless we are talking to old client + */

fix this comment, please

+ if (eof_identifier && + (thd->client_capabilities & CLIENT_DEPRECATE_EOF)) + buff[0]= 254; + else + buff[0]= 0;

I don't understand this, because the comment doesn't explain anything

+ + /* affected rows */ + pos= net_store_length(buff + 1, affected_rows); + + /* last insert id */ + pos= net_store_length(pos, id); + + if ((thd->client_capabilities & CLIENT_SESSION_TRACK) && + thd->session_tracker.enabled_any() && + thd->session_tracker.changed_any()) + { + server_status |= SERVER_SESSION_STATE_CHANGED; + state_changed= true;

I'd rather have trackers to set server_status when they're changed, instead of iterating the list of trackers here twice for every statement just because we apparently have free CPU cycles to burn

+ } + if (thd->client_capabilities & CLIENT_PROTOCOL_41) { DBUG_PRINT("info", @@ -247,9 +282,45 @@ net_send_ok(THD *thd, } thd->get_stmt_da()->set_overwrite_status(true);

+ if ((thd->client_capabilities & CLIENT_SESSION_TRACK)) + { + /* the info field */ + if (state_changed || (message && message[0])) + pos= net_store_data(pos, (uchar*) message, message ? strlen(message) : 0); + /* session state change information */ + if (unlikely(state_changed)) + { + store.set_charset(thd->variables.collation_database); + + /* + First append the fields collected so far. In case of malloc, memory + for message is also allocated here. + */ + store.append((const char *)start, (pos - start), MYSQL_ERRMSG_SIZE);

no, don't use "buf or store, depending on whether session tracking is used" that's ridiculous (and an extra memcpy) always use store, remove buf. declare store as StringBuffer<MYSQL_ERRMSG_SIZE+10> store(thd->variables.collation_database);

+ + /* .. and then the state change information. */ + thd->session_tracker.store(thd, store); + + start= (uchar *) store.ptr(); + pos= start+store.length(); + } + } - if (message && message[0]) + else if (message && message[0]) + { + /* the info field, if there is a message to store */ pos= net_store_data(pos, (uchar*) message, strlen(message)); - error= my_net_write(net, buff, (size_t) (pos-buff)); + } + + /* OK packet length will be restricted to 16777215 bytes */ + if (((size_t) (pos - start)) > MAX_PACKET_LENGTH) + { + net->error= 1;

I'm surprised, this doesn't have a comment "setting net->error to 1" would be quite in style with what I've seen so far :)

+ 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); + } + error= my_net_write(net, start, (size_t) (pos - start)); if (!error) error= net_flush(net);

@@ -559,7 +630,20 @@ bool Protocol::send_ok(uint server_status, uint statement_warn_count, bool Protocol::send_eof(uint server_status, uint statement_warn_count) { DBUG_ENTER("Protocol::send_eof"); - const bool retval= net_send_eof(thd, server_status, statement_warn_count); + bool retval; + /* + Normally end of statement reply is signaled by OK packet, but in case + of binlog dump request an EOF packet is sent instead. Also, old clients + expect EOF packet instead of OK + */ +#ifndef EMBEDDED_LIBRARY + if ((thd->client_capabilities & CLIENT_DEPRECATE_EOF) && + (thd->get_command() != COM_BINLOG_DUMP )) + retval= net_send_ok(thd, server_status, statement_warn_count, 0, 0, NULL, + true); + else +#endif + retval= net_send_eof(thd, server_status, statement_warn_count);

better keep the old code here, just net_send_eof(). and in net_send_eof() you do, like if (thd->client_capabilities & CLIENT_DEPRECATE_EOF) return net_send_ok(..., 0, 0, NULL, true);

DBUG_RETURN(retval); }

@@ -1551,8 +1641,14 @@ bool Protocol_binary::send_out_parameters(List<Item_param> *sp_params) /* Restore THD::server_status. */ thd->server_status&= ~SERVER_PS_OUT_PARAMS;

- /* Send EOF-packet. */ - net_send_eof(thd, thd->server_status, 0); + if (thd->client_capabilities & CLIENT_DEPRECATE_EOF) + ret= net_send_ok(thd, + (thd->server_status | SERVER_PS_OUT_PARAMS | + SERVER_MORE_RESULTS_EXISTS), + 0, 0, 0, NULL, true); + else + /* In case of old clients send EOF packet. */ + ret= net_send_eof(thd, thd->server_status, 0);

/* Reset SERVER_MORE_RESULTS_EXISTS bit, because this is the last packet

note that unlike 5.7 you we have this "Reset SERVER_MORE_RESULTS_EXISTS" *after* net_send_eof() (bugfix for MDEV-4604) 1. you don't need "| SERVER_MORE_RESULTS_EXISTS" for net_send_ok() 2. you might try to use only net_send_eof() here too (with both flags set)

diff --git a/sql/session_tracker.h b/sql/session_tracker.h new file mode 100644 index 0000000..7477f96 --- /dev/null +++ b/sql/session_tracker.h @@ -0,0 +1,255 @@ +#ifndef SESSION_TRACKER_INCLUDED +#define SESSION_TRACKER_INCLUDED + +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved. + + 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 */

why not to use the same enum from mysql_com.h ?

+}; + +#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().

this is illogical. check() and update() do not belong in the base State_tracker

+*/ + +class State_tracker +{ +protected: + /** Is tracking enabled for a particular session state type ? */ + 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; +}; + + +/** + 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. */ + /** Copy constructor */ + Session_tracker(Session_tracker const &other) + { + DBUG_ASSERT(FALSE); + } + + /** Copy assignment operator */ + Session_tracker& operator= (Session_tracker const &rhs) + { + DBUG_ASSERT(FALSE); + return *this; + } + +public:

judging by other comments in this file (above and below), I'm surprised this keyword does not have a comment, like /* declare the rest of the object public. that makes the methods below accessible from the other code, not only from this object itself */ Serisouly, why do these comments explain basic C++ features? or repeat whatever the function is obviously doing (like, get_tracker() or enabled_any()) And non-trivial functions are not commented, for example server_boot_verify()

+ + /** Constructor */ + Session_tracker() + {} + + /** Destructor */ + ~Session_tracker() + { + } + /** + Initialize Session_tracker objects and enable them based on the + tracker_xxx variables' value that the session inherit from global + variables at the time of session initialization (see plugin_thdvar_init). + */ + void init(const CHARSET_INFO *char_set); + void enable(THD *thd); + bool server_boot_verify(const CHARSET_INFO *char_set, LEX_STRING var_list); + + /** Returns the pointer to the tracker object for the specified tracker. */ + State_tracker *get_tracker(enum_session_tracker tracker) const; + + /** Checks if m_enabled flag is set for any of the tracker objects. */ + bool enabled_any(); + + /** Checks if m_changed flag is set for any of the tracker objects. */ + bool changed_any(); + + /** + Stores the session state change information of all changes session state + type entities into the specified buffer. + */ + void store(THD *thd, String &main_buf); + void deinit() + { + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + delete m_trackers[i]; + } +}; + +/* + 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 +*/ + +class Session_state_change_tracker : public State_tracker

Why is this one in the header, not in session_tracker.cc?

+{ +private: + + void reset(); + +public: + Session_state_change_tracker(); + bool enable(THD *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) + {} +}; + + +/** + Transaction_state_tracker + ---------------------- + This is a tracker class that enables & manages the tracking of + current transaction info for a particular connection. +*/ + +/** + 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 + 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 +};

Better remove all declarations related to transaction tracking and add them later in a separate patch (with the complete transaction tracking feature).

+ +#endif /* SESSION_TRACKER_INCLUDED */ diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc new file mode 100644 index 0000000..6abff2a --- /dev/null +++ b/sql/session_tracker.cc @@ -0,0 +1,454 @@ +/* Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.

and "Copyright(c) 2016, MariaDB" because it's not 1:1 copy

+ + 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 "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 "xa.h"

this is for SESSION_TRACK_TRANSACTION_STATE too I suppose

+ +static void store_lenenc_string(String &to, const char *from, + size_t length); + +class Dummy_tracker : public State_tracker +{ +bool enable(THD *thd __attribute__((unused)))

why __attribute__((unused)) if it is used? and why is it used, if the tracker is dummy? btw, seeing how this Dummy_tracker is used, I'd rather rename it to Not_implemented_tracker

+ { return update(thd); } + bool check(THD *thd __attribute__((unused)), + set_var *var __attribute__((unused))) + { return false; }

it's C++, you don't need __attribute__((unused)), write bool check(THD *, set_var *) { return false; }

+ bool update(THD *thd __attribute__((unused))) + { return false; } + bool store(THD *thd __attribute__((unused)), + String &buf __attribute__((unused))) + + { return false; } + void mark_as_changed(THD *thd __attribute__((unused)), + LEX_CSTRING *tracked_item_name __attribute__((unused))) + {} + +}; + + +/** + 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: + + /** Constructor */ + 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); +}; + +/* To be used in expanding the buffer. */ +static const unsigned int EXTRA_ALLOC= 1024; + +/////////////////////////////////////////////////////////////////////////////// + +/** + @brief 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)? true: false; + return false; +} + + +/** + @brief Store the schema name 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. + + @return + false Success + true Error +*/ + +bool Current_schema_tracker::store(THD *thd, String &buf) +{ + ulonglong db_length, length; + + length= db_length= thd->db_length; + length += net_length_size(length); + + uchar *to= (uchar *) buf.prep_append(net_length_size(length) + 1, + EXTRA_ALLOC); + + /* Session state type (SESSION_TRACK_SCHEMA) */ + to= net_store_length(to, (ulonglong)SESSION_TRACK_SCHEMA); + + /* Length of the overall entity. */ + to= net_store_length(to, length); + + /* Length of the changed current schema name. */ + net_store_length(to, db_length); + + /* Current schema name (length-encoded string). */ + store_lenenc_string(buf, thd->db, thd->db_length); + + reset(); + + return false; +} + + +/** + @brief Mark the tracker as changed. + + @param name [IN] Always null. + + @return void +*/ + +void Current_schema_tracker::mark_as_changed(THD *thd, + LEX_CSTRING *tracked_item_name + __attribute__((unused)))

you don't need __attribute__((unused)) here, it's C++ besides, as I've just found, we compile with -Wno-unused-parameter (this was merged from MySQL, perhaps we should remove that flag)

+{ + m_changed= true; + thd->lex->safe_to_cache_query= 0; +} + + +/** + @brief Reset the m_changed flag for next statement. + + @return void +*/ + +void Current_schema_tracker::reset() +{ + m_changed= false; +} + + +/////////////////////////////////////////////////////////////////////////////// + +/** Constructor */ +Session_state_change_tracker::Session_state_change_tracker() +{ + m_changed= false; +} + +/** + @brief Initiate the value of m_enabled based on + @@session_track_state_change value. + + @param thd [IN] The thd handle. + @return false (always) + +**/ + +bool Session_state_change_tracker::enable(THD *thd) +{ + m_enabled= (thd->variables.session_track_state_change)? true: false; + return 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) +{ + return enable(thd); +}

you're kidding, right? For Dummy_tracker and Current_schema_tracker enable() calls upate(), for Session_state_change_tracker update() calls enable(). and no matter what method calls what, it's always one_method(THD *thd) { return other_method(thd); } other_method(THD *thd) { m_enabled= (thd->variables.session_track_XXX)? true: false; } why two methods enable/update and not one? why "? true : false", and not m_enabled=thd->variables.session_track_XXX (hint: m_enabled is bool, ?true:false is the same as cast to bool anyway) and why to copy the value in the first place??? Just use the value from thd->variables.session_track_XXX

+ +/** + @brief Store the 1byte boolean flag in the specified buffer. Once the + data is stored, we reset the flags related to state-change. If + 1byte flag valie is 1 then there is a session state change else + there is no state change information. + + @param thd [IN] The thd handle. + @paran buf [INOUT] Buffer to store the information to. + + @return + false Success + true Error +**/ + +bool Session_state_change_tracker::store(THD *thd, String &buf) +{ + /* since its a boolean tracker length is always 1 */ + const ulonglong length= 1; + + uchar *to= (uchar *) buf.prep_append(3,EXTRA_ALLOC); + + /* format of the payload is as follows: + [ tracker type] [length] [1 byte flag] */

Noooo! Do not copy the payload format logic to every tracker. Keep it in one place (preferrably, in the protocol)

+ /* Session state type (SESSION_TRACK_STATE_CHANGE) */ + to= net_store_length(to, (ulonglong)SESSION_TRACK_STATE_CHANGE); + + /* Length of the overall entity it is always 1 byte */ + to= net_store_length(to, length); + + /* boolean tracker will go here */ + *to= (is_state_changed(thd) ? '1' : '0'); + + reset(); + + return false; +} + +/** + @brief Mark the tracker as changed and associated session + attributes accordingly. + + @param name [IN] Always null. + @return void +*/ + +void Session_state_change_tracker::mark_as_changed(THD *thd, + LEX_CSTRING *tracked_item_name) +{ + /* do not send the boolean flag for the tracker itself + in the OK packet */ + if(tracked_item_name && + (strncmp(tracked_item_name->str, "session_track_state_change", 26) == 0)) + m_changed= false;

Grrr. 1. Why not? 2. what if it is SET @foobar=5, @@session_track_state_change=ON;

+ else + { + m_changed= true; + thd->lex->safe_to_cache_query= 0;

why?

+ } +} + +/** + @brief Reset the m_changed flag for next statement. + + @return void +*/ + +void Session_state_change_tracker::reset() +{ + m_changed= false; +} + +/** + @brief find if there is a session state change + + @return + true - if there is a session state change + false - if there is no session state change +**/ + +bool Session_state_change_tracker::is_state_changed(THD* thd) +{ + return m_changed; +} + +/////////////////////////////////////////////////////////////////////////////// + +/** + @brief Initialize session tracker objects. + + @param char_set [IN] The character set info. + + @return void +*/ + +void Session_tracker::init(const CHARSET_INFO *char_set) +{ + m_trackers[SESSION_SYSVARS_TRACKER]= + new (std::nothrow) Dummy_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) Dummy_tracker; + m_trackers[TRANSACTION_INFO_TRACKER]= + new (std::nothrow) Dummy_tracker; +} + +/** + @brief Enables the tracker objects. + + @param thd [IN] The thread handle. + + @return void +*/ +void Session_tracker::enable(THD *thd) +{ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + m_trackers[i]->enable(thd); +} + +/** + @brief Method called during the server startup to verify the contents + of @@session_track_system_variables.

I see. Please remove it from this patch. It should've been added in the next (sysvar) patch, but as it's a wrong approach in general, I hope we'll do it correctly.

+ + @param char_set The character set info. + @param var_list Value of @@session_track_system_variables. + + @return false Success + true failure +*/ +bool Session_tracker::server_boot_verify(const CHARSET_INFO *char_set, + LEX_STRING var_list) +{ + return false; +} + + + +/** + @brief Returns the pointer to the tracker object for the specified tracker. + + @param tracker [IN] Tracker type. + + @return Pointer to the tracker object. +*/ + +State_tracker * +Session_tracker::get_tracker(enum_session_tracker tracker) const +{ + return m_trackers[tracker]; +}

make that inline in session_tracker.h

+ + +/** + @brief Checks if m_enabled flag is set for any of the tracker objects. + + @return + true - At least one of the trackers is enabled. + false - None of the trackers is enabled. + +*/ + +bool Session_tracker::enabled_any() +{ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + { + if (m_trackers[i]->is_enabled()) + return true; + } + return false; +}

remove enabled_any() and changed_any(). See my comments where these methods are used.

+ +/** + @brief Checks if m_changed flag is set for any of the tracker objects. + + @return + true At least one of the entities being tracker has + changed. + false None of the entities being tracked has changed. +*/ + +bool Session_tracker::changed_any() +{ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + { + if (m_trackers[i]->is_changed()) + return true; + } + return false; +} + + +/** + @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. + + @return void +*/ + +void Session_tracker::store(THD *thd, String &buf) +{ + /* Temporary buffer to store all the changes. */ + String temp; + size_t length; + + /* Get total length. */ + for (int i= 0; i <= SESSION_TRACKER_END; i ++) + { + if (m_trackers[i]->is_changed()) + m_trackers[i]->store(thd, temp); + } + + length= temp.length(); + /* Store length first.. */ + char *to= buf.prep_append(net_length_size(length), EXTRA_ALLOC); + net_store_length((uchar *) to, length); + + /* .. and then the actual info. */ + buf.append(temp);

Nice. Copy all data into a temporary malloced buffer, them memcopy it again into the malloced real buffer. At least use StringBuffer here. it would be good to avoid memcpy too, but I only see two possible approaches, both not perfect: 1. assume it's, say, less than 251 byte in the most common case. put the data directly into buf, starting from buf.ptr() + 1. when done, put the length into the reserved byte. If the length is more than 251 - memmove everything to free more bytes. 2. iterate all session trackers twice. first - to get the length, second - to put the actual data ah, yes, and don't use references (String &buf) use pointers (String *buf)

+} + + +/** + @brief Stores the given string in length-encoded format into the specified + buffer. + + @param to [IN] Buffer to store the given string in. + @param from [IN] The give string to be stored. + @param length [IN] Length of the above string. + + @return void. +*/ + +static +void store_lenenc_string(String &to, const char *from, size_t length)

why not make it a method of String ? Like, append_lenenc() ?

+{ + char *ptr; + ptr= to.prep_append(net_length_size(length), EXTRA_ALLOC); + net_store_length((uchar *) ptr, length); + to.append(from, length); +} + diff --git a/sql/set_var.cc b/sql/set_var.cc index b5430c5..a55fc54 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -199,8 +199,24 @@ 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) + { + if ((!ret) && + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, &var->var->name);

don't check for is_enabled here. Alternatives: 1. just do mark_as_changed unconditionally. this works if mark_as_changed is cheap and has no side-effects. neither is true now, but I don't understand why. 2. create a wrapper method, like Session_tracker::mark_as_changed(tracker) at least you won't need to copy this long line in every place where a state changes. doing 1 is preferrable, but I don't know if it's easily possible (because I don't understand why it's done that way now)

+ } + + return ret; + } }

uchar *sys_var::session_value_ptr(THD *thd, const LEX_STRING *base) @@ -965,6 +983,8 @@ int set_var_collation_client::update(THD *thd) thd->variables.character_set_results= character_set_results; thd->variables.collation_connection= collation_connection; thd->update_charset(); + if (thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, NULL); thd->protocol_text.init(thd); thd->protocol_binary.init(thd); return 0;

what about set_var_password::update and set_var_role::update ? Wouldn't it be better to mark_as_changed in sql_set_variables instead of doing that in individual sys_var descendands?

diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 0d168e9..933f060 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7154,3 +7154,8 @@ ER_WRONG_ORDER_IN_WITH_CLAUSE eng "The definition of the table '%s' refers to the table '%s' defined later in a non-recursive WITH clause" ER_RECURSIVE_QUERY_IN_WITH_CLAUSE eng "Recursive queries in WITH clause are not supported yet" +ER_DUP_LIST_ENTRY + eng "Duplicate entry '%-.192s'."

really, a bug? remove this. having the same value twice is not a bug: MariaDB (test)> set sql_mode='ansi,ansi'; Query OK, 0 rows affected (0.00 sec) Let's be consistent

+ER_NET_OK_PACKET_TOO_LARGE 08S01 + eng "OK packet too large" + ukr "Пакет OK надто великий" diff --git a/sql/sp_head.cc b/sql/sp_head.cc index d58b51a..6205dd6 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -2975,6 +2975,13 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp,

reinit_stmt_before_use(thd, m_lex);

+#ifndef EMBEDDED_LIBRARY

why?

+ if ((thd->client_capabilities & CLIENT_SESSION_TRACK) && + thd->session_tracker.enabled_any() && + thd->session_tracker.changed_any()) + thd->lex->safe_to_cache_query= 0;

why?

+#endif + if (open_tables) res= instr->exec_open_and_lock_tables(thd, m_lex->query_tables);

diff --git a/sql/sql_class.cc b/sql/sql_class.cc index e3b7056..bf70594 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1461,6 +1461,11 @@ void THD::init(void) /* Initialize the Debug Sync Facility. See debug_sync.cc. */ debug_sync_init_thread(this); #endif /* defined(ENABLED_DEBUG_SYNC) */ + + /* Initialize session_tracker and create all tracker objects */ + session_tracker.init(this->charset()); + session_tracker.enable(this);

1. you don't need charset here, it's unused 2. thus you don't need to invoke init() explicitly at all, do it from the Session_tracker constructor. In fact, move init functionality into the constructor.

+ apc_target.init(&LOCK_thd_data); DBUG_VOID_RETURN; } @@ -1620,6 +1625,12 @@ void THD::cleanup(void) /* All metadata locks must have been released by now. */ DBUG_ASSERT(!mdl_context.has_locks());

+ /* + Destroy trackers only after finishing manipulations with transaction + state to avoid issues with Transaction_state_tracker. + */ + session_tracker.deinit();

may be you can do that from the destructor too

+ apc_target.destroy(); cleanup_done=1; DBUG_VOID_RETURN; diff --git a/sql/sql_class.h b/sql/sql_class.h index be652e6..063198e 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -681,6 +682,11 @@ typedef struct system_variables

my_bool pseudo_slave_mode;

+ char *track_sysvars_ptr;

no track_sysvars_ptr in this patch please

+ my_bool session_track_schema; + my_bool session_track_state_change; + ulong session_track_transaction_info;

and no session_track_transaction_info either

+ } SV;

/** diff --git a/sql/sql_db.cc b/sql/sql_db.cc index 2ba67cb..53719e5 100644 --- a/sql/sql_db.cc +++ b/sql/sql_db.cc @@ -1034,7 +1034,18 @@ 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); + { + mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);\

eh? trailing backslash?

+ /* + Check if current database tracker is enabled. If so, set the 'changed' flag. + */ + if (thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->is_enabled()) + { + LEX_CSTRING dummy= { C_STRING_WITH_LEN("") }; + dummy.length= dummy.length*1;

WAT???

+ thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->mark_as_changed(thd, &dummy);

alternatively, make mark_as_changed to take const char* and size_t, then you won't need to create LEX_CSTRING alternatively, create LEX_CSTRING dummy statically and use it always alternatively, use empty_lex_string btw, what's the point of using an *empty string* as an argument? other trackers accept NULL. why empty string is better? and anyway, why to construct and pass down an argument that is ignored anyway???

+ } + } my_dirend(dirp); DBUG_RETURN(error); } @@ -1588,6 +1597,18 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)

mysql_change_db_impl(thd, &new_db_file_name, db_access, db_default_cl);

+done: + /* + Check if current database tracker is enabled. If so, set the 'changed' flag. + */ + if (thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->is_enabled()) + { + LEX_CSTRING dummy= { C_STRING_WITH_LEN("") }; + dummy.length= dummy.length*1; + thd->session_tracker.get_tracker(CURRENT_SCHEMA_TRACKER)->mark_as_changed(thd, &dummy);

same

+ } + if (thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, NULL); DBUG_RETURN(FALSE); }

diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index dbe1967..244ff5b 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -322,6 +322,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)

remove that

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); @@ -2776,22 +2778,23 @@ static void update_func_double(THD *thd, struct st_mysql_sys_var *var, System Variables support ****************************************************************************/

- -sys_var *find_sys_var(THD *thd, const char *str, uint length) +sys_var *find_sys_var_ex(THD *thd, const char *str, size_t length, + bool throw_error, bool locked)

revert all sql_plugin.cc and sql_plugin.h changes. if you need them for sysvars - do them in the second patch

{ sys_var *var; sys_var_pluginvar *pi= NULL; plugin_ref plugin; - DBUG_ENTER("find_sys_var"); + DBUG_ENTER("find_sys_var_ex");

- mysql_mutex_lock(&LOCK_plugin); + if (!locked) + mysql_mutex_lock(&LOCK_plugin); mysql_rwlock_rdlock(&LOCK_system_variables_hash); if ((var= intern_find_sys_var(str, length)) && (pi= var->cast_pluginvar())) { mysql_rwlock_unlock(&LOCK_system_variables_hash); LEX *lex= thd ? thd->lex : 0; - if (!(plugin= intern_plugin_lock(lex, plugin_int_to_ref(pi->plugin)))) + if (!(plugin= my_intern_plugin_lock(lex, plugin_int_to_ref(pi->plugin)))) var= NULL; /* failed to lock it, it must be uninstalling */ else if (!(plugin_state(plugin) & PLUGIN_IS_READY)) diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 8e5ab71..03f3a93 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -2755,7 +2755,13 @@ void mysql_sql_stmt_prepare(THD *thd) thd->stmt_map.erase(stmt); } else + { + /* send the boolean tracker in the OK packet when

rewrite the comment into something that makes sense, please

+ @@session_track_state_change is set to ON */ + if (thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->is_enabled()) + thd->session_tracker.get_tracker(SESSION_STATE_CHANGE_TRACKER)->mark_as_changed(thd, NULL); my_ok(thd, 0L, 0L, "Statement prepared"); + }

DBUG_VOID_RETURN; }

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