Bug#19704825 TEMPORARY SLAVE TYPE CONVERSION TABLES RELEASED TO EARLY

Problem: The memory used in preparing slave type conversion temporary table
is getting released early and causing unexpected results
      
Analysis: As part of bug#18770469 fix, We introduced an event
m_event_mem_root (a special mem root), added to Log_event
class. While server is creating the temporary table, the memory needed
is allocated from this special mem root which will be freed in ~Log_event()
i.e., the scope of this memroot is one event. But it could happen
that in some cases, server might need to access this
conversion temporary table for next following event. 
For eg: A nested row event (insert is causing insert in a trigger)
In this situation, the memory is getting delayed too early 
and causing issues when the server is trying to access the temporary
table inside the trigger.
      
Fix: We cannot use a mem_root whose scope is limited to an event
execution in this situation. With some further analysis, found out
that clearing a thd->mem_root at the end of statement (upon applying
an event which has STMT_END_F flag) will solve out of memory problem
while running a long transactions (bug#18770469) and will also
make this reported problem (memory is getting released early) to go away.

Author: Venkatesh Duggirala

mysql-test/suite/rpl/r/rpl_colSize.result

@@ -185,8 +185,22 @@ t1	CREATE TABLE `t1` (
   `d` bit(25) DEFAULT NULL,
   `e` bit(13) DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
+CREATE TABLE t2(i INTEGER);
+CREATE TRIGGER t1_tr BEFORE INSERT ON t1 FOR EACH ROW
+BEGIN
+INSERT INTO t2 VALUES(1);
+END $$
+INSERT INTO t1 VALUES (
+b'1010101',
+b'10101011',
+b'101010110101010101111',
+b'10101010101',
+b'10101011111'
+  );
+include/sync_slave_sql_with_master.inc
 *** Cleanup  ***
 DROP TABLE t1;
+DROP TABLE t2;
 include/sync_slave_sql_with_master.inc
 SET GLOBAL SLAVE_TYPE_CONVERSIONS = @saved_slave_type_conversions;
 include/rpl_end.inc

mysql-test/suite/rpl/t/rpl_colSize.test

@@ -215,16 +215,63 @@ SHOW CREATE TABLE t1;
 --let $sync_slave_connection= slave
 --source include/sync_slave_sql_with_master.inc
 
-
-
 --echo Insert some values and select them on master
 SELECT BIN(a), BIN(b), BIN(c), BIN(d), BIN(e) FROM t1;
 --replace_result default DEFAULT
 SHOW CREATE TABLE t1;
 
+###############################################################################
+# Bug#19704825 TEMPORARY SLAVE TYPE CONVERSION TABLES RELEASED TO EARLY
+# Problem: Memory used in preparing slave type conversion
+# temporary table is getting released early and causing unexpected results
+# in case of nested events.
+# Eg: If a insert statement is calling a trigger which contains another
+# insert statement.
+# Steps to reproduce:
+#  1) Create a table t1
+#  2) Change it's definition on slave, so that insert will create conversion
+#     temporary table on slave (set SLAVE_TYPE_CONVERSIONS appropriately to
+#     allow the scenario)
+#  3) Create a table t2
+#  4) Create a trigger on top of table t1. Inside the trigger, insert a tuple
+#      into table t2
+#  5) Insert a tuple into table t1 on Master
+#  6) Make sure this insert is replicated without any issues.
+###############################################################################
+
+# Step 1, 2 are already done by above test code.
+
+# Step 3: Create a table t2
+connection master;
+CREATE TABLE t2(i INTEGER);
+
+# Step 4: Create a trigger on top of table t1. Inside the trigger, insert a
+#          tuple into table t2
+--delimiter $$
+CREATE TRIGGER t1_tr BEFORE INSERT ON t1 FOR EACH ROW
+BEGIN
+INSERT INTO t2 VALUES(1);
+END $$
+--delimiter ;
+
+# Step 5: Insert a tuple into table t1 on Master
+INSERT INTO t1 VALUES (
+  b'1010101',
+  b'10101011',
+  b'101010110101010101111',
+  b'10101010101',
+  b'10101011111'
+  );
+
+# Step 6) Make sure this insert is replicated without any issues.
+--source include/sync_slave_sql_with_master.inc
+
+# End of test for Bug#19704825
+
 --echo *** Cleanup  ***
 connection master;
 DROP TABLE t1;
+DROP TABLE t2;
 --source include/sync_slave_sql_with_master.inc
 
 SET GLOBAL SLAVE_TYPE_CONVERSIONS = @saved_slave_type_conversions;

sql/field.cc

@@ -10167,8 +10167,7 @@ Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length,
 		  Field::geometry_type geom_type,
 		  Field::utype unireg_check,
 		  TYPELIB *interval,
-		  const char *field_name,
-		  MEM_ROOT *mem_root /* default= NULL */)
+		  const char *field_name)
 {
   uchar *UNINIT_VAR(bit_ptr);
   uchar UNINIT_VAR(bit_offset);
@@ -10203,8 +10202,6 @@ Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length,
                        FLAGSTR(pack_flag, FIELDFLAG_NUMBER),
                        FLAGSTR(pack_flag, FIELDFLAG_PACK),
                        FLAGSTR(pack_flag, FIELDFLAG_BLOB)));
-  if (mem_root == NULL)
-    mem_root= current_thd->mem_root;
 
   if (f_is_alpha(pack_flag))
   {
@@ -10213,11 +10210,11 @@ Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length,
       if (field_type == MYSQL_TYPE_STRING ||
           field_type == MYSQL_TYPE_DECIMAL ||   // 3.23 or 4.0 string
           field_type == MYSQL_TYPE_VAR_STRING)
-        return new (mem_root) Field_string(ptr,field_length,null_pos,null_bit,
+        return new Field_string(ptr,field_length,null_pos,null_bit,
                                 unireg_check, field_name,
                                 field_charset);
       if (field_type == MYSQL_TYPE_VARCHAR)
-        return new (mem_root) Field_varstring(ptr,field_length,
+        return new Field_varstring(ptr,field_length,
                                    HA_VARCHAR_PACKLENGTH(field_length),
                                    null_pos,null_bit,
                                    unireg_check, field_name,
@@ -10232,115 +10229,115 @@ Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length,
 
 #ifdef HAVE_SPATIAL
     if (f_is_geom(pack_flag))
-      return new (mem_root) Field_geom(ptr,null_pos,null_bit,
+      return new Field_geom(ptr,null_pos,null_bit,
 			    unireg_check, field_name, share,
 			    pack_length, geom_type);
 #endif
     if (f_is_blob(pack_flag))
-      return new (mem_root) Field_blob(ptr,null_pos,null_bit,
+      return new Field_blob(ptr,null_pos,null_bit,
 			    unireg_check, field_name, share,
 			    pack_length, field_charset);
     if (interval)
     {
       if (f_is_enum(pack_flag))
-	return new (mem_root) Field_enum(ptr,field_length,null_pos,null_bit,
+	return new Field_enum(ptr,field_length,null_pos,null_bit,
 				  unireg_check, field_name,
 				  pack_length, interval, field_charset);
       else
-	return new (mem_root) Field_set(ptr,field_length,null_pos,null_bit,
+	return new Field_set(ptr,field_length,null_pos,null_bit,
 			     unireg_check, field_name,
 			     pack_length, interval, field_charset);
     }
   }
 
   switch (field_type) {
   case MYSQL_TYPE_DECIMAL:
-    return new (mem_root) Field_decimal(ptr,field_length,null_pos,null_bit,
+    return new Field_decimal(ptr,field_length,null_pos,null_bit,
 			     unireg_check, field_name,
 			     f_decimals(pack_flag),
 			     f_is_zerofill(pack_flag) != 0,
 			     f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_NEWDECIMAL:
-    return new (mem_root) Field_new_decimal(ptr,field_length,null_pos,null_bit,
+    return new Field_new_decimal(ptr,field_length,null_pos,null_bit,
                                  unireg_check, field_name,
                                  f_decimals(pack_flag),
                                  f_is_zerofill(pack_flag) != 0,
                                  f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_FLOAT:
-    return new (mem_root) Field_float(ptr,field_length,null_pos,null_bit,
+    return new Field_float(ptr,field_length,null_pos,null_bit,
 			   unireg_check, field_name,
 			   f_decimals(pack_flag),
 			   f_is_zerofill(pack_flag) != 0,
 			   f_is_dec(pack_flag)== 0);
   case MYSQL_TYPE_DOUBLE:
-    return new (mem_root) Field_double(ptr,field_length,null_pos,null_bit,
+    return new Field_double(ptr,field_length,null_pos,null_bit,
 			    unireg_check, field_name,
 			    f_decimals(pack_flag),
 			    f_is_zerofill(pack_flag) != 0,
 			    f_is_dec(pack_flag)== 0);
   case MYSQL_TYPE_TINY:
-    return new (mem_root) Field_tiny(ptr,field_length,null_pos,null_bit,
+    return new Field_tiny(ptr,field_length,null_pos,null_bit,
 			  unireg_check, field_name,
 			  f_is_zerofill(pack_flag) != 0,
 			  f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_SHORT:
-    return new (mem_root) Field_short(ptr,field_length,null_pos,null_bit,
+    return new Field_short(ptr,field_length,null_pos,null_bit,
 			   unireg_check, field_name,
 			   f_is_zerofill(pack_flag) != 0,
 			   f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_INT24:
-    return new (mem_root) Field_medium(ptr,field_length,null_pos,null_bit,
+    return new Field_medium(ptr,field_length,null_pos,null_bit,
 			    unireg_check, field_name,
 			    f_is_zerofill(pack_flag) != 0,
 			    f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_LONG:
-    return new (mem_root) Field_long(ptr,field_length,null_pos,null_bit,
+    return new Field_long(ptr,field_length,null_pos,null_bit,
 			   unireg_check, field_name,
 			   f_is_zerofill(pack_flag) != 0,
 			   f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_LONGLONG:
-    return new (mem_root) Field_longlong(ptr,field_length,null_pos,null_bit,
+    return new Field_longlong(ptr,field_length,null_pos,null_bit,
 			      unireg_check, field_name,
 			      f_is_zerofill(pack_flag) != 0,
 			      f_is_dec(pack_flag) == 0);
   case MYSQL_TYPE_TIMESTAMP:
-    return new (mem_root) Field_timestamp(ptr, field_length, null_pos, null_bit,
+    return new Field_timestamp(ptr, field_length, null_pos, null_bit,
                                unireg_check, field_name);
   case MYSQL_TYPE_TIMESTAMP2:
-    return new (mem_root) Field_timestampf(ptr, null_pos, null_bit,
+    return new Field_timestampf(ptr, null_pos, null_bit,
                                 unireg_check, field_name,
                                 field_length > MAX_DATETIME_WIDTH ?
                                 field_length - 1 - MAX_DATETIME_WIDTH : 0);
   case MYSQL_TYPE_YEAR:
-    return new (mem_root) Field_year(ptr,field_length,null_pos,null_bit,
+    return new Field_year(ptr,field_length,null_pos,null_bit,
 			  unireg_check, field_name);
   case MYSQL_TYPE_NEWDATE:
-    return new (mem_root) Field_newdate(ptr, null_pos, null_bit, unireg_check, field_name);
+    return new Field_newdate(ptr, null_pos, null_bit, unireg_check, field_name);
 
   case MYSQL_TYPE_TIME:
-    return new (mem_root) Field_time(ptr, null_pos, null_bit,
+    return new Field_time(ptr, null_pos, null_bit,
                           unireg_check, field_name);
   case MYSQL_TYPE_TIME2:
-    return new (mem_root) Field_timef(ptr, null_pos, null_bit,
+    return new Field_timef(ptr, null_pos, null_bit,
                            unireg_check, field_name, 
                            (field_length > MAX_TIME_WIDTH) ?
                            field_length - 1 - MAX_TIME_WIDTH : 0);
   case MYSQL_TYPE_DATETIME:
-    return new (mem_root) Field_datetime(ptr, null_pos, null_bit,
+    return new Field_datetime(ptr, null_pos, null_bit,
                               unireg_check, field_name);
   case MYSQL_TYPE_DATETIME2:
-    return new (mem_root) Field_datetimef(ptr, null_pos, null_bit,
+    return new Field_datetimef(ptr, null_pos, null_bit,
                                unireg_check, field_name,
                                (field_length > MAX_DATETIME_WIDTH) ?
                                field_length - 1 - MAX_DATETIME_WIDTH : 0);
   case MYSQL_TYPE_NULL:
-    return new (mem_root) Field_null(ptr, field_length, unireg_check, field_name,
+    return new Field_null(ptr, field_length, unireg_check, field_name,
                           field_charset);
   case MYSQL_TYPE_BIT:
     return f_bit_as_char(pack_flag) ?
-           new (mem_root) Field_bit_as_char(ptr, field_length, null_pos, null_bit,
+           new Field_bit_as_char(ptr, field_length, null_pos, null_bit,
                                  unireg_check, field_name) :
-           new (mem_root) Field_bit(ptr, field_length, null_pos, null_bit, bit_ptr,
+           new Field_bit(ptr, field_length, null_pos, null_bit, bit_ptr,
                          bit_offset, unireg_check, field_name);
 
   default:					// Impossible (Wrong version)

sql/field.h

@@ -3893,8 +3893,7 @@ Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length,
 		  const CHARSET_INFO *cs,
 		  Field::geometry_type geom_type,
 		  Field::utype unireg_check,
-		  TYPELIB *interval, const char *field_name,
-		  MEM_ROOT *mem_root= NULL);
+		  TYPELIB *interval, const char *field_name);
 uint pack_length_to_packflag(uint type);
 enum_field_types get_blob_type_from_length(ulong length);
 uint32 calc_pack_length(enum_field_types type,uint32 length);

sql/log_event.cc

@@ -727,9 +727,6 @@ Log_event::Log_event(THD* thd_arg, uint16 flags_arg,
   server_id= thd->server_id;
   unmasked_server_id= server_id;
   when= thd->start_time;
-#ifdef HAVE_REPLICATION
-  init_sql_alloc(&m_event_mem_root, 4096, 0);
-#endif //HAVE_REPLICATION
 }
 
 /**
@@ -754,9 +751,6 @@ Log_event::Log_event(enum_event_cache_type cache_type_arg,
   when.tv_sec=  0;
   when.tv_usec= 0;
   log_pos=	0;
-#ifdef HAVE_REPLICATION
-  init_sql_alloc(&m_event_mem_root, 4096, 0);
-#endif //HAVE_REPLICATION
 }
 #endif /* !MYSQL_CLIENT */
 
@@ -774,11 +768,7 @@ Log_event::Log_event(const char* buf,
 {
 #ifndef MYSQL_CLIENT
   thd = 0;
-#ifdef HAVE_REPLICATION
-  init_sql_alloc(&m_event_mem_root, 4096, 0);
-#endif //HAVE_REPLICATION
 #endif
-
   when.tv_sec= uint4korr(buf);
   when.tv_usec= 0;
   server_id = uint4korr(buf + SERVER_ID_OFFSET);
@@ -9910,8 +9900,7 @@ Rows_log_event::decide_row_lookup_algorithm_and_key()
   this->m_key_index= search_key_in_table(table, cols, (PRI_KEY_FLAG | UNIQUE_KEY_FLAG | MULTIPLE_KEY_FLAG));
   this->m_rows_lookup_algorithm= ROW_LOOKUP_HASH_SCAN;
   if (m_key_index < MAX_KEY)
-    m_distinct_key_spare_buf= (uchar*) alloc_root(&m_event_mem_root,
-                                                  table->key_info[m_key_index].key_length);
+    m_distinct_key_spare_buf= (uchar*) thd->alloc(table->key_info[m_key_index].key_length);
   DBUG_PRINT("info", ("decide_row_lookup_algorithm_and_key: decided - HASH_SCAN"));
   goto end;
 
@@ -10442,8 +10431,7 @@ Rows_log_event::add_key_to_distinct_keyset()
   if (ret.second)
   {
     /* Insert is successful, so allocate a new buffer for next key */
-    m_distinct_key_spare_buf= (uchar*) alloc_root(&m_event_mem_root,
-                                                  m_key_info->key_length);
+    m_distinct_key_spare_buf= (uchar*) thd->alloc(m_key_info->key_length);
     if (!m_distinct_key_spare_buf)
     {
       error= HA_ERR_OUT_OF_MEM;
@@ -11153,13 +11141,8 @@ int Rows_log_event::do_apply_event(Relay_log_info const *rli)
       {
         DBUG_ASSERT(ptr->m_tabledef_valid);
         TABLE *conv_table;
-        /*
-          Use special mem_root 'Log_event::m_event_mem_root' while doing
-          compatiblity check (i.e., while creating temporary table)
-         */
         if (!ptr->m_tabledef.compatible_with(thd, const_cast<Relay_log_info*>(rli),
-                                             ptr->table,
-                                             &conv_table,&m_event_mem_root))
+                                             ptr->table, &conv_table))
         {
           DBUG_PRINT("debug", ("Table: %s.%s is not compatible with master",
                                ptr->table->s->db.str,
@@ -11438,13 +11421,26 @@ int Rows_log_event::do_apply_event(Relay_log_info const *rli)
     DBUG_RETURN(error);
   }
 
-  if (get_flags(STMT_END_F) && (error= rows_event_stmt_cleanup(rli, thd)))
+  if (get_flags(STMT_END_F))
+  {
+   if((error= rows_event_stmt_cleanup(rli, thd)))
     slave_rows_error_report(ERROR_LEVEL,
                             thd->is_error() ? 0 : error,
                             rli, thd, table,
                             get_type_str(),
                             const_cast<Relay_log_info*>(rli)->get_rpl_log_name(),
                             (ulong) log_pos);
+   /* We are at end of the statement (STMT_END_F flag), lets clean
+     the memory which was used from thd's mem_root now.
+     This needs to be done only if we are here in SQL thread context.
+     In other flow ( in case of a regular thread which can happen
+     when the thread is applying BINLOG'...' row event) we should
+     *not* try to free the memory here. It will be done latter
+     in dispatch_command() after command execution is completed.
+    */
+   if (thd->slave_thread)
+     free_root(thd->mem_root, MYF(MY_KEEP_PREALLOC));
+  }
   DBUG_RETURN(error);
 }
 

sql/log_event.h

@@ -1347,13 +1347,7 @@ class Log_event
   }
   Log_event(const char* buf, const Format_description_log_event
             *description_event);
-  virtual ~Log_event()
-  {
-    free_temp_buf();
-#if !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION)
-    free_root(&m_event_mem_root, MYF(MY_KEEP_PREALLOC));
-#endif //!MYSQL_CLIENT && HAVE_REPLICATION
-  }
+  virtual ~Log_event() { free_temp_buf();}
   void register_temp_buf(char* buf) { temp_buf = buf; }
   void free_temp_buf()
   {
@@ -1609,13 +1603,6 @@ class Log_event
 
   virtual int do_apply_event_worker(Slave_worker *w);
 
-  /*
-    Mem root whose scope is equalent to event's scope.
-    This mem_root will be initialized in constructor
-    Log_event() and freed in destructor ~Log_event().
-   */
-  MEM_ROOT m_event_mem_root;
-
 protected:
 
   /**