On 07.04.2016 17:03, Sergei Golubchik wrote:
> 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.
Ok (general comment), sthat issue with buffers is really  something slip 
out from me. Everything else I just I tried keep it as close to the 
original code is possible to make future merge easier in general. If it 
is not an issue I just have my head eaching to fix comments ane 
formating and some other parts.
> 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
Not completely (dealing with SET STATEMENT postponded in hope of better 
test abilities, and also I hope to get OK or not OK to whole new code.
>
>> +  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
Some thought it can be useful (do not remember who told it)
>> +  SESSION_TRACK_TRANSACTION_CHARACTERISTICS,  /* Transaction chistics */
>> +  SESSION_TRACK_TRANSACTION_STATE             /* Transaction state */
> to be imlemented later, I suppose
Yes, but need to get more info about our transactions first.
>
>> +};
>> +
>> +#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
I took them as "at least something we can test now", And I had no idea 
that that test  obsolete (why they are not removed?)
>
>> +#
>> +# 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.
ok
>
>>    
>>     @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
OK
>
>> +  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
OK
>
>> +  }
>> +
>>     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 :)
yes, but I explained why I kept all that ...
>> +    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);
OK
>
>>     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)
OK
>
>> 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 ?
It is different, last position corresponds to 2 last in the other (one 
tracker, but 2 type of information from it).
>
>> +};
>> +
>> +#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?
probably it was needed out, as well as transaction tracker... I'll check 
(transactional is for sure for its additional methods, probably other 
man was writing it).

Whould it be better just make empty metods for other tracker to avoid 
some tracking seen outside as here?
>
>> +{
>> +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).
OK
>> +
>> +#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
OK
>
>> +  { 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
Dommy was made apon schema but other, yes was "crossed" :)
>
>> +
>> +/**
>> +  @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;
hehe, OK I'll rewrite
>
>> +  else
>> +  {
>> +    m_changed= true;
>> +    thd->lex->safe_to_cache_query= 0;
> why?
Because it add info to the packet, QC store packet as is, so reply from 
the QC could be mislieading. In fact it do not limit QC a lot because it 
is unusual when SELECT change something. It is more safety for SELECTS 
with side effects (which should not be cached in any case).
>
>> +  }
>> +}
>> +
>> +/**
>> +  @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.
OK
>
>> +
>> +  @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
OK
>
>> +
>> +
>> +/**
>> +  @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)
OK, I'll rewrite it
>
>> +}
>> +
>> +
>> +/**
>> +  @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() ?
OK
>> +{
>> +  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)
OK
>> +    }
>> +
>> +    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?
OK
>
>> 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
If we could allow this incompatibility, I'd also would vote for it.
>
>> +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?
because all this is not supported by embedded
>
>> +  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?
see above, ot one of things thay did right IMHO.
>
>> +#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.
I removed chagset in next patch.

It was needed for names of variables in hash, I do not store names (but 
addresses) so I do not need it any more.
>
>> +
>>     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
I'll try.
>
>> +
>>     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
it was removed completely, sorry.
>
>> +  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?
mistyping I have not noticed, sorry.
>
>> +    /*
>> +      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???
  +1 should be. But OK, I'll fix that dummy at all.
>
>> +      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???
yes yes, null is way better here.
>
>> +    }
>> +  }
>>     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
OK
>>   {
>>     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
OK
>
>> +       @@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
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp