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(a)mariadb.org
