Bug#13693012: SLAVE CRASHING ON INSERT STATEMENT WITH MERGE TABLE

PROBLEM: After WL 4144, when using MyISAM Merge tables, the routine
open_and_lock_tables will append to the list of tables to lock, the
base tables that make up the MERGE table. This has two side-effects in
replication: 

  1. On the master side, we log additional table maps for the base
     tables, since they appear in the list of locked tables, even
     though we don't really use them at the slave.

  2. On the slave side, when opening a MERGE table while applying a
     ROW event, additional tables are appended to the list of tables
     to lock.

Side-effect #1 is not harmful. It's just that when using MyISAM Merge
tables a few table maps more may be logged.

Side-effect #2, is harmful, because the list rli->tables_to_lock is an
extended structure from TABLE_LIST in which the extra fields are
filled from the table maps that are processed. Since
open_and_lock_tables appends tables to the list after all table map
events have been processed we end up with entries without
replication/table map data on them. Thus when trying to access that
info for these extra tables, the server will crash.

SOLUTION: We fix side-effect #2 by making sure that we access the
replication part of the structure for those in the list that were
accounted for when processing the correspondent table map events. All
in all, we never go beyond rli->tables_to_lock_count.

We also deploy an assertion when clearing rli->tables_to_lock, making
sure that the base tables are not in the list anymore (were closed in
close_thread_tables).

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

+include/master-slave.inc
+[connection master]
+CREATE TABLE t1 (a int) ENGINE=MyISAM;
+CREATE TABLE t2 (a int) ENGINE=MyISAM;
+INSERT INTO t1 VALUES (1), (2), (3);
+INSERT INTO t2 VALUES (4), (5), (6);
+CREATE TABLE IF NOT EXISTS t1_merge LIKE t1;
+ALTER TABLE t1_merge ENGINE=MERGE UNION (t2, t1);
+include/diff_tables.inc [master:test.t1, slave:test.t1]
+include/diff_tables.inc [master:test.t2, slave:test.t2]
+UPDATE t1_merge SET a=10 WHERE a=1;
+DELETE FROM t1_merge WHERE a=10;
+include/diff_tables.inc [master:test.t1, slave:test.t1]
+include/diff_tables.inc [master:test.t2, slave:test.t2]
+DROP TABLE t1_merge, t1, t2;
+include/rpl_end.inc

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

+#
+# BUG#47103
+# 
+# This test case checks whether the slave crashes or not when there is
+# a merge table in use.
+#
+# Description
+# ===========
+#
+# The test case creates two regular MyISAM tables on the master and
+# one MERGE table. Then it populates the MyISAM tables, updates and 
+# deletes their contents through the merge table. Finally, the slave
+# is synchronized with the master and (after the fix) it won't crash.
+# 
+--source include/master-slave.inc
+--source include/have_binlog_format_row.inc
+--connection master
+
+CREATE TABLE t1 (a int) ENGINE=MyISAM;
+CREATE TABLE t2 (a int) ENGINE=MyISAM;
+INSERT INTO t1 VALUES (1), (2), (3);
+INSERT INTO t2 VALUES (4), (5), (6);
+CREATE TABLE IF NOT EXISTS t1_merge LIKE t1;
+ALTER TABLE t1_merge ENGINE=MERGE UNION (t2, t1);
+
+--sync_slave_with_master
+
+--let diff_tables=master:test.t1, slave:test.t1
+--source include/diff_tables.inc
+
+--let diff_tables=master:test.t2, slave:test.t2
+--source include/diff_tables.inc
+
+--connection master
+UPDATE t1_merge SET a=10 WHERE a=1;
+DELETE FROM t1_merge WHERE a=10;
+
+--sync_slave_with_master
+--connection master
+
+--let diff_tables=master:test.t1, slave:test.t1
+--source include/diff_tables.inc
+
+--let diff_tables=master:test.t2, slave:test.t2
+--source include/diff_tables.inc
+
+DROP TABLE t1_merge, t1, t2;
+--sync_slave_with_master
+
+--source include/rpl_end.inc

sql/log_event.cc

@@ -7641,9 +7641,24 @@ int Rows_log_event::do_apply_event(Relay_log_info const *rli)
     {
       DBUG_PRINT("debug", ("Checking compability of tables to lock - tables_to_lock: %p",
                            rli->tables_to_lock));
+
+      /**
+        When using RBR and MyISAM MERGE tables the base tables that make
+        up the MERGE table can be appended to the list of tables to lock.
+  
+        Thus, we just check compatibility for those that tables that have
+        a correspondent table map event (ie, those that are actually going
+        to be accessed while applying the event). That's why the loop stops
+        at rli->tables_to_lock_count .
+
+        NOTE: The base tables are added here are removed when 
+              close_thread_tables is called.
+       */
       RPL_TABLE_LIST *ptr= rli->tables_to_lock;
-      for ( ; ptr ; ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_global))
+      for (uint i= 0 ; ptr && (i < rli->tables_to_lock_count);
+           ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_global), i++)
       {
+        DBUG_ASSERT(ptr->m_tabledef_valid);
         TABLE *conv_table;
         if (!ptr->m_tabledef.compatible_with(thd, const_cast<Relay_log_info*>(rli),
                                              ptr->table, &conv_table))
@@ -7681,10 +7696,10 @@ int Rows_log_event::do_apply_event(Relay_log_info const *rli)
       Rows_log_event, we can invalidate the query cache for the
       associated table.
      */
-    for (TABLE_LIST *ptr= rli->tables_to_lock ; ptr ; ptr= ptr->next_global)
-    {
+    TABLE_LIST *ptr= rli->tables_to_lock;
+    for (uint i=0 ;  ptr && (i < rli->tables_to_lock_count); ptr= ptr->next_global, i++)
       const_cast<Relay_log_info*>(rli)->m_table_map.set_table(ptr->table_id, ptr->table);
-    }
+
 #ifdef HAVE_QUERY_CACHE
     query_cache.invalidate_locked_for_write(rli->tables_to_lock);
 #endif
@@ -8466,9 +8481,9 @@ check_table_map(Relay_log_info const *rli, RPL_TABLE_LIST *table_list)
     res= FILTERED_OUT;
   else
   {
-    for(RPL_TABLE_LIST *ptr= static_cast<RPL_TABLE_LIST*>(rli->tables_to_lock);
-        ptr; 
-        ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_local))
+    RPL_TABLE_LIST *ptr= static_cast<RPL_TABLE_LIST*>(rli->tables_to_lock);
+    for(uint i=0 ; ptr && (i< rli->tables_to_lock_count); 
+        ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_local), i++)
     {
       if (ptr->table_id == table_list->table_id)
       {

sql/log_event_old.cc

@@ -126,8 +126,10 @@ Old_rows_log_event::do_apply_event(Old_rows_log_event *ev, const Relay_log_info
 
     {
       RPL_TABLE_LIST *ptr= rli->tables_to_lock;
-      for ( ; ptr ; ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_global))
+      for (uint i= 0 ; ptr&& (i< rli->tables_to_lock_count); 
+           ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_global), i++)
       {
+        DBUG_ASSERT(ptr->m_tabledef_valid);
         TABLE *conv_table;
         if (!ptr->m_tabledef.compatible_with(thd, const_cast<Relay_log_info*>(rli),
                                              ptr->table, &conv_table))
@@ -158,10 +160,9 @@ Old_rows_log_event::do_apply_event(Old_rows_log_event *ev, const Relay_log_info
       Old_rows_log_event, we can invalidate the query cache for the
       associated table.
      */
-    for (TABLE_LIST *ptr= rli->tables_to_lock ; ptr ; ptr= ptr->next_global)
-    {
+    TABLE_LIST *ptr= rli->tables_to_lock;
+    for (uint i=0; ptr && (i < rli->tables_to_lock_count); ptr= ptr->next_global, i++)
       const_cast<Relay_log_info*>(rli)->m_table_map.set_table(ptr->table_id, ptr->table);
-    }
 #ifdef HAVE_QUERY_CACHE
     query_cache.invalidate_locked_for_write(rli->tables_to_lock);
 #endif
@@ -1539,7 +1540,8 @@ int Old_rows_log_event::do_apply_event(Relay_log_info const *rli)
 
     {
       RPL_TABLE_LIST *ptr= rli->tables_to_lock;
-      for ( ; ptr ; ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_global))
+      for (uint i= 0 ; ptr&& (i< rli->tables_to_lock_count);
+           ptr= static_cast<RPL_TABLE_LIST*>(ptr->next_global), i++)
       {
         TABLE *conv_table;
         if (ptr->m_tabledef.compatible_with(thd, const_cast<Relay_log_info*>(rli),

sql/rpl_rli.cc

@@ -1264,6 +1264,23 @@ void Relay_log_info::cleanup_context(THD *thd, bool error)
 
 void Relay_log_info::clear_tables_to_lock()
 {
+  DBUG_ENTER("Relay_log_info::clear_tables_to_lock()");
+#ifndef DBUG_OFF
+  /**
+    When replicating in RBR and MyISAM Merge tables are involved
+    open_and_lock_tables (called in do_apply_event) appends the 
+    base tables to the list of tables_to_lock. Then these are 
+    removed from the list in close_thread_tables (which is called 
+    before we reach this point).
+
+    This assertion just confirms that we get no surprises at this
+    point.
+   */
+  uint i=0;
+  for (TABLE_LIST *ptr= tables_to_lock ; ptr ; ptr= ptr->next_global, i++) ;
+  DBUG_ASSERT(i == tables_to_lock_count);
+#endif  
+
   while (tables_to_lock)
   {
     uchar* to_free= reinterpret_cast<uchar*>(tables_to_lock);
@@ -1288,10 +1305,12 @@ void Relay_log_info::clear_tables_to_lock()
     my_free(to_free);
   }
   DBUG_ASSERT(tables_to_lock == NULL && tables_to_lock_count == 0);
+  DBUG_VOID_RETURN;
 }
 
 void Relay_log_info::slave_close_thread_tables(THD *thd)
 {
+  DBUG_ENTER("Relay_log_info::slave_close_thread_tables(THD *thd)");
   thd->stmt_da->can_overwrite_status= TRUE;
   thd->is_error() ? trans_rollback_stmt(thd) : trans_commit_stmt(thd);
   thd->stmt_da->can_overwrite_status= FALSE;
@@ -1313,5 +1332,6 @@ void Relay_log_info::slave_close_thread_tables(THD *thd)
     thd->mdl_context.release_statement_locks();
 
   clear_tables_to_lock();
+  DBUG_VOID_RETURN;
 }
 #endif