BUG#19667258: WL#6972: SERVER CRASH ON SET SESSION SESSION_TRACK_GTID…

…S='OFF'

The server crashes when changing the value of session_track_gtids from
'ALL_GTIDS' or 'OWN_GTID' to 'OFF'. This was due to the fact that the
memory structures were being released to early in the process.

We fix this by deferring the internal memory structures cleanup until
after the OK packet has been filled with the required information.

Furthermore, this patch fixes the behavior regarding the GTID
information that is put in the OK packet during transitions between
states of session_track_gtids, i.e., when the user issues 
 
  SET session_track_gtids='...'.

The behavior is the following during transitions (from -> to):

  |-----------+-----------------------------------|
  |    FROM   |                TO                 |
  |-----------+-----------+-----------+-----------|
  |           | OFF       | OWN_GTID  | ALL_GTIDS |
  |-----------+-----------+-----------+-----------|
  | OFF       | No GTID   | No GTID   | No GTID   |
  | OWN_GTID  | No GTID   | No GTID   | No GTID   |
  | ALL_GTIDS | ALL_GTIDS | ALL_GTIDS | ALL_GTIDS |
  |-----------+-----------+-----------+-----------|

This patch also includes tests for the various transitions depicted in
the above table.

mysql-test/suite/binlog/r/binlog_gtid_mix_ok_packet_all_gtids.result

@@ -0,0 +1,46 @@
+# ALL_GTIDS at startup: this must return GTID:1
+CREATE TABLE t1 (c1 INT) Engine=InnoDB;
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1
+
+# ALL_GTIDS at startup: this must return GTID:1-2
+DROP TABLE t1;
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-2
+
+## ALL_GTIDS -> ALL_GTIDS : this must return GTID:1-2 ##
+SET SESSION session_track_gtids='ALL_GTIDS';
+SET SESSION session_track_gtids='ALL_GTIDS';
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-2
+
+## ALL_GTIDS -> OWN_GTID: this must return GTID:1-2 ##
+SET SESSION session_track_gtids='ALL_GTIDS';
+SET SESSION session_track_gtids='OWN_GTID';
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-2
+
+## ALL_GTIDS -> OFF : this must return GTID:1-2 ##
+SET SESSION session_track_gtids='ALL_GTIDS';
+SET SESSION session_track_gtids='OFF';
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-2
+
+## OWN_GTID -> ALL_GTIDS : this must return '' ##
+SET SESSION session_track_gtids='OWN_GTID';
+SET SESSION session_track_gtids='ALL_GTIDS';
+## OWN_GTID -> OWN_GTID: this must return '' ##
+SET SESSION session_track_gtids='OWN_GTID';
+SET SESSION session_track_gtids='OWN_GTID';
+## OWN_GTID -> OFF : this must return '' ##
+SET SESSION session_track_gtids='OWN_GTID';
+SET SESSION session_track_gtids='OFF';
+## OFF -> ALL_GTIDS : this must return '' ##
+SET SESSION session_track_gtids='OFF';
+SET SESSION session_track_gtids='ALL_GTIDS';
+## OFF -> OWN_GTID: this must return '' ##
+SET SESSION session_track_gtids='OFF';
+SET SESSION session_track_gtids='OWN_GTID';
+## OFF -> OFF : this must return '' ##
+SET SESSION session_track_gtids='OFF';
+SET SESSION session_track_gtids='OFF';

mysql-test/suite/binlog/r/binlog_gtid_mix_response_packet.result

@@ -1,3 +1,30 @@
+CREATE TABLE t1 (c1 INT) Engine=InnoDB;
+DROP TABLE t1;
+set session session_track_gtids='own_gtid';
+CREATE TABLE t1 (c1 INT) Engine=InnoDB;
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:3
+
+INSERT INTO t1 VALUES (1);
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:4
+
+set session session_track_gtids='all_gtids';
+SELECT * FROM t1;
+c1
+1
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-4
+
+DROP TABLE t1;
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-5
+
+set session session_track_gtids='off';
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-5
+
+RESET MASTER;
 SELECT @@server_uuid;
 @@server_uuid
 00000000-1111-0000-1111-000000000000
@@ -13,10 +40,6 @@ CREATE TABLE t2 (c1 INT) Engine=InnoDB;
 
 SET SESSION GTID_NEXT=AUTOMATIC;
 SET SESSION SESSION_TRACK_GTIDS=ALL_GTIDS;
--- Tracker : SESSION_TRACK_GTIDS
--- 11111111-aaaa-2222-bbbb-000000000000:1,
-11111111-aaaa-2222-bbbb-111111111111:1
-
 CREATE TABLE t3 (c1 INT) Engine=InnoDB;
 -- Tracker : SESSION_TRACK_GTIDS
 -- 00000000-1111-0000-1111-000000000000:1,
@@ -71,6 +94,9 @@ DROP TABLE t1, t2;
 -- 00000000-1111-0000-1111-000000000000:1-6
 
 SET SESSION SESSION_TRACK_GTIDS=OWN_GTID;
+-- Tracker : SESSION_TRACK_GTIDS
+-- 00000000-1111-0000-1111-000000000000:1-6
+
 RESET MASTER;
 CREATE TABLE t1 (c1 INT) Engine=InnoDB;
 -- Tracker : SESSION_TRACK_GTIDS

mysql-test/suite/binlog/t/binlog_gtid_mix_ok_packet_all_gtids-master.opt

@@ -0,0 +1 @@
+--log-slave-updates --enforce-gtid-consistency --gtid-mode=ON --debug=+d,server_uuid_deterministic --session-track-gtids=ALL_GTIDS

mysql-test/suite/binlog/t/binlog_gtid_mix_ok_packet_all_gtids.test

@@ -0,0 +1,71 @@
+--source include/have_binlog_format_mixed.inc
+--source include/have_gtid.inc
+--source include/have_debug.inc
+
+# BUG#19667258: WL#6972 - SERVER CRASH ON SET SESSION SESSION_TRACK_GTIDS='OFF'
+
+# This test file is configured to start the server with:
+#
+#  --session_track_gtids='ALL_GTIDS'
+#
+# Then we are looking to assert the following:
+# - Verify that the following statements include
+#   GTID data in the OK packet immediately after start
+#   the server starts up.
+# - Verify that every transition includes the expected
+#   GTID data in the OK packet.
+
+--enable_session_track_info
+
+--echo # ALL_GTIDS at startup: this must return GTID:1
+CREATE TABLE t1 (c1 INT) Engine=InnoDB;
+
+--echo # ALL_GTIDS at startup: this must return GTID:1-2
+DROP TABLE t1;
+
+--disable_session_track_info
+
+--let $i=3
+while($i)
+{
+  if ($i==3)
+  {
+    --let $from=ALL_GTIDS
+  }
+
+  if ($i==2)
+  {
+    --let $from=OWN_GTID
+  }
+
+  if ($i==1)
+  {
+    --let $from=OFF
+  }
+
+  --let $text= this must return ''
+  if (`SELECT '$from' = 'ALL_GTIDS'`)
+  {
+    --let $text= this must return GTID:1-2
+  }
+
+  --echo ## $from -> ALL_GTIDS : $text ##
+  --eval SET SESSION session_track_gtids='$from'
+  --enable_session_track_info
+  SET SESSION session_track_gtids='ALL_GTIDS';
+  --disable_session_track_info
+
+  --echo ## $from -> OWN_GTID: $text ##
+  --eval SET SESSION session_track_gtids='$from'
+  --enable_session_track_info
+  SET SESSION session_track_gtids='OWN_GTID';
+  --disable_session_track_info
+
+  --echo ## $from -> OFF : $text ##
+  --eval SET SESSION session_track_gtids='$from'
+  --enable_session_track_info
+  SET SESSION session_track_gtids='OFF';
+  --disable_session_track_info
+
+  --dec $i
+}

mysql-test/suite/binlog/t/binlog_gtid_mix_response_packet.test

@@ -8,6 +8,21 @@
 # need debug build to be able to set a deterministic server uuid
 --source include/have_debug.inc
 
+# BUG#19667258: WL#6972 - SERVER CRASH ON SET SESSION SESSION_TRACK_GTIDS='OFF'
+
+--enable_session_track_info
+CREATE TABLE t1 (c1 INT) Engine=InnoDB;
+DROP TABLE t1;
+set session session_track_gtids='own_gtid';
+CREATE TABLE t1 (c1 INT) Engine=InnoDB;
+INSERT INTO t1 VALUES (1);
+set session session_track_gtids='all_gtids';
+SELECT * FROM t1;
+DROP TABLE t1;
+set session session_track_gtids='off';
+RESET MASTER;
+--disable_session_track_info
+
 ########################################################
 # Sanity Test.
 #

sql/rpl_context.cc

@@ -55,7 +55,7 @@ bool Session_consistency_gtids_ctx::notify_after_transaction_commit(const THD* t
   if (!shall_collect(thd))
     DBUG_RETURN(res);
 
-  if (thd->variables.session_track_gtids == ALL_GTIDS)
+  if (m_curr_session_track_gtids == ALL_GTIDS)
   {
     /*
      If one is configured to read all writes, we always collect
@@ -84,7 +84,7 @@ bool Session_consistency_gtids_ctx::notify_after_gtid_executed_update(const THD
   if (!shall_collect(thd))
     DBUG_RETURN(res);
 
-  if (thd->variables.session_track_gtids == OWN_GTID)
+  if (m_curr_session_track_gtids == OWN_GTID)
   {
     const Gtid& gtid= thd->owned_gtid;
     if (gtid.sidno == -1) // we need to add thd->owned_gtid_set
@@ -131,12 +131,19 @@ bool Session_consistency_gtids_ctx::notify_after_response_packet(const THD *thd)
   if (m_gtid_set && !m_gtid_set->is_empty())
     m_gtid_set->clear();
 
+  /*
+   Every time we get a notification that a packet was sent, we update 
+   this value. It may have changed (the previous command may have been
+   a SET SESSION session_track_gtids=...;).
+   */
+  m_curr_session_track_gtids= thd->variables.session_track_gtids;
   DBUG_RETURN(res);
 }
 
 void
 Session_consistency_gtids_ctx::register_ctx_change_listener(
-              Session_consistency_gtids_ctx::Ctx_change_listener* listener)
+              Session_consistency_gtids_ctx::Ctx_change_listener* listener,
+              THD* thd)
 {
   DBUG_ASSERT(m_listener == NULL || m_listener == listener);
   if (m_listener == NULL)
@@ -145,6 +152,13 @@ Session_consistency_gtids_ctx::register_ctx_change_listener(
     m_listener= listener;
     m_sid_map= new Sid_map(NULL);
     m_gtid_set= new Gtid_set(m_sid_map);
+
+    /*
+     Caches the value at startup if needed. This is called during THD::init,
+     if the session_track_gtids value is set at startup time to anything 
+     different than OFF.
+     */
+    m_curr_session_track_gtids= thd->variables.session_track_gtids;
   }
 }
 

sql/rpl_context.h

@@ -75,6 +75,17 @@ class Session_consistency_gtids_ctx
   */
   Session_consistency_gtids_ctx::Ctx_change_listener* m_listener;
 
+  /**
+   Keeps track of the current session track gtids, so that we capture
+   according to what was set before. For instance, if the user does:
+   SET @@SESSION.SESSION_TRACK_GTIDS='ALL_GTIDS';
+   ...
+   SET @@SESSION.SESSION_TRACK_GTIDS='OWN_GTID';
+
+   The last statement should return a set of GTIDs.
+  */
+  ulong m_curr_session_track_gtids;
+
 protected:
 
   /*
@@ -111,15 +122,19 @@ class Session_consistency_gtids_ctx
    Registers the listener. The pointer MUST not be NULL.
 
    @param listener a pointer to the listener to register.
+   @param thd THD context associated to this listener.
   */
-  void register_ctx_change_listener(Session_consistency_gtids_ctx::Ctx_change_listener* listener);
+  void register_ctx_change_listener(
+    Session_consistency_gtids_ctx::Ctx_change_listener* listener,
+    THD* thd);
 
   /**
    Unregisters the listener. The listener MUST have registered previously.
 
    @param listener a pointer to the listener to register.
   */
-  void unregister_ctx_change_listener(Session_consistency_gtids_ctx::Ctx_change_listener* listener);
+  void unregister_ctx_change_listener(
+    Session_consistency_gtids_ctx::Ctx_change_listener* listener);
 
   /**
     This member function MUST return a reference to the set of collected

sql/session_tracker.cc

@@ -1157,7 +1157,7 @@ bool Session_gtids_tracker::update(THD *thd)
   if (m_enabled)
   {
     // register to listen to gtids context state changes
-    thd->rpl_thd_ctx.session_gtids_ctx().register_ctx_change_listener(this);
+    thd->rpl_thd_ctx.session_gtids_ctx().register_ctx_change_listener(this, thd);
 
     // instantiate the encoder if needed
     if (m_encoder == NULL)
@@ -1174,15 +1174,7 @@ bool Session_gtids_tracker::update(THD *thd)
       m_encoder= new Session_gtids_ctx_encoder_string();
     }
   }
-  else // break the bridge between tracker and collector
-  {
-    /* No need to listen to gtids context state changes */
-    thd->rpl_thd_ctx.session_gtids_ctx().unregister_ctx_change_listener(this);
-
-    // delete the encoder (just to free memory)
-    delete m_encoder; // if not tracking, delete the encoder
-    m_encoder= NULL;
-  }
+  // else /* break the bridge between tracker and collector */
   return false;
 }
 
@@ -1202,10 +1194,9 @@ bool Session_gtids_tracker::update(THD *thd)
 
 bool Session_gtids_tracker::store(THD *thd, String &buf)
 {
-  if (m_encoder->encode(thd, buf))
+  if (m_encoder && m_encoder->encode(thd, buf))
     return true;
-  if (!buf.is_empty())
-    reset();
+  reset();
   return false;
 }
 
@@ -1232,5 +1223,18 @@ void Session_gtids_tracker::mark_as_changed(LEX_CSTRING *tracked_item_name
 
 void Session_gtids_tracker::reset()
 {
+  /*
+   Delete the encoder and remove the listener if this had been previously
+   deactivated.
+   */
+  if (!m_enabled && m_encoder)
+  {
+    /* No need to listen to gtids context state changes */
+    current_thd->rpl_thd_ctx.session_gtids_ctx().unregister_ctx_change_listener(this);
+
+    // delete the encoder (just to free memory)
+    delete m_encoder; // if not tracking, delete the encoder
+    m_encoder= NULL;
+  }
   m_changed= false;
-}
+}