Bug#42438: Crash ha_partition::change_table_ptr

There was two problems:
The first was the symptom, caused by bad error handling in
ha_partition. It did not handle print_error etc. when
having no partitions (when used by dummy handler).

The second was the real problem that when dropping tables
it reused the table type (storage engine) from when the lock
was asked for, not the table type that it had when gaining
the exclusive name lock. So that it tried to delete tables
from wrong storage engines.

Solutions for the first problem was to accept some handler
calls to the partitioning handler even if it was not setup
with any partitions, and also if possible fallback
to use the base handler's default functions.

Solution for the second problem was to remove the optimization
to reuse the definition from the cache, instead always check
the frm-file when holding the LOCK_open mutex

(updated with a fix for a debug print crash and better
comments as required by reviewer, and removed optimization
to avoid reading the frm-file).

5 files changed, 198 insertions, 37 deletions

diff --git a/mysql-test/r/partition_debug_sync.result b/mysql-test/r/partition_debug_sync.result
new file mode 100644
index 00000000000..5eb19f42395
--- /dev/null
+++ b/mysql-test/r/partition_debug_sync.result
@@ -0,0 +1,57 @@
+DROP TABLE IF EXISTS t1, t2;
+SET DEBUG_SYNC= 'RESET';
+#
+# Bug#42438: Crash ha_partition::change_table_ptr
+# Test when remove partitioning is done while drop table is waiting
+# for the table.
+# Con 1
+SET DEBUG_SYNC= 'RESET';
+CREATE TABLE t1
+(a INTEGER,
+b INTEGER NOT NULL,
+KEY (b))
+ENGINE = MYISAM
+/*!50100  PARTITION BY RANGE (a)
+(PARTITION p0 VALUES LESS THAN (2),
+PARTITION p1 VALUES LESS THAN (20),
+PARTITION p2 VALUES LESS THAN (100),
+PARTITION p3 VALUES LESS THAN MAXVALUE ) */;
+SET DEBUG_SYNC= 'alter_table_before_create_table_no_lock SIGNAL removing_partitioning WAIT_FOR waiting_for_alter';
+SET DEBUG_SYNC= 'alter_table_before_main_binlog SIGNAL partitioning_removed';
+ALTER TABLE t1 REMOVE PARTITIONING;
+# Con default
+SET DEBUG_SYNC= 'now WAIT_FOR removing_partitioning';
+SET DEBUG_SYNC= 'waiting_for_table SIGNAL waiting_for_alter';
+SET DEBUG_SYNC= 'rm_table_part2_before_delete_table WAIT_FOR partitioning_removed';
+DROP TABLE IF EXISTS t1;
+# Con 1
+SET DEBUG_SYNC= 'RESET';
+SET DEBUG_SYNC= 'RESET';
+#
+# Bug#42438: Crash ha_partition::change_table_ptr
+# Test when remove partitioning is failing due to drop table is already
+# in progress.
+CREATE TABLE t2
+(a INTEGER,
+b INTEGER NOT NULL,
+KEY (b))
+ENGINE = MYISAM
+/*!50100  PARTITION BY RANGE (a)
+(PARTITION p0 VALUES LESS THAN (2),
+PARTITION p1 VALUES LESS THAN (20),
+PARTITION p2 VALUES LESS THAN (100),
+PARTITION p3 VALUES LESS THAN MAXVALUE ) */;
+SET DEBUG_SYNC= 'before_lock_tables_takes_lock SIGNAL removing_partitions WAIT_FOR waiting_for_alter';
+SET DEBUG_SYNC= 'alter_table_before_rename_result_table WAIT_FOR delete_done';
+ALTER TABLE t2 REMOVE PARTITIONING;
+# Con default
+SET DEBUG_SYNC= 'now WAIT_FOR removing_partitions';
+SET DEBUG_SYNC= 'waiting_for_table SIGNAL waiting_for_alter';
+SET DEBUG_SYNC= 'rm_table_part2_before_binlog SIGNAL delete_done';
+DROP TABLE IF EXISTS t2;
+# Con 1
+ERROR 42S02: Table 'test.t2' doesn't exist
+SET DEBUG_SYNC= 'RESET';
+# Con default
+SET DEBUG_SYNC= 'RESET';
+End of 5.1 tests
diff --git a/mysql-test/t/partition_debug_sync.test b/mysql-test/t/partition_debug_sync.test
new file mode 100644
index 00000000000..69bc86a8bb7
--- /dev/null
+++ b/mysql-test/t/partition_debug_sync.test
@@ -0,0 +1,81 @@
+#--disable_abort_on_error
+#
+# Test for the partition storage engine which require DEBUG_SYNC feature to
+# Created by Mattias Jonsson
+#
+--source include/have_partition.inc
+--source include/have_debug_sync.inc
+
+--disable_warnings
+DROP TABLE IF EXISTS t1, t2;
+SET DEBUG_SYNC= 'RESET';
+--enable_warnings
+
+--echo #
+--echo # Bug#42438: Crash ha_partition::change_table_ptr
+--echo # Test when remove partitioning is done while drop table is waiting
+--echo # for the table.
+connect(con1, localhost, root,,);
+--echo # Con 1
+SET DEBUG_SYNC= 'RESET';
+CREATE TABLE t1
+(a INTEGER,
+ b INTEGER NOT NULL,
+ KEY (b))
+ENGINE = MYISAM
+/*!50100  PARTITION BY RANGE (a)
+(PARTITION p0 VALUES LESS THAN (2),
+ PARTITION p1 VALUES LESS THAN (20),
+ PARTITION p2 VALUES LESS THAN (100),
+ PARTITION p3 VALUES LESS THAN MAXVALUE ) */;
+SET DEBUG_SYNC= 'alter_table_before_create_table_no_lock SIGNAL removing_partitioning WAIT_FOR waiting_for_alter';
+SET DEBUG_SYNC= 'alter_table_before_main_binlog SIGNAL partitioning_removed';
+--send ALTER TABLE t1 REMOVE PARTITIONING
+connection default;
+--echo # Con default
+SET DEBUG_SYNC= 'now WAIT_FOR removing_partitioning';
+SET DEBUG_SYNC= 'waiting_for_table SIGNAL waiting_for_alter';
+SET DEBUG_SYNC= 'rm_table_part2_before_delete_table WAIT_FOR partitioning_removed';
+DROP TABLE IF EXISTS t1;
+--echo # Con 1
+connection con1;
+--reap
+connection default;
+SET DEBUG_SYNC= 'RESET';
+connection con1;
+SET DEBUG_SYNC= 'RESET';
+
+--echo #
+--echo # Bug#42438: Crash ha_partition::change_table_ptr
+--echo # Test when remove partitioning is failing due to drop table is already
+--echo # in progress.
+CREATE TABLE t2
+(a INTEGER,
+ b INTEGER NOT NULL,
+ KEY (b))
+ENGINE = MYISAM
+/*!50100  PARTITION BY RANGE (a)
+(PARTITION p0 VALUES LESS THAN (2),
+ PARTITION p1 VALUES LESS THAN (20),
+ PARTITION p2 VALUES LESS THAN (100),
+ PARTITION p3 VALUES LESS THAN MAXVALUE ) */;
+SET DEBUG_SYNC= 'before_lock_tables_takes_lock SIGNAL removing_partitions WAIT_FOR waiting_for_alter';
+SET DEBUG_SYNC= 'alter_table_before_rename_result_table WAIT_FOR delete_done';
+--send ALTER TABLE t2 REMOVE PARTITIONING
+connection default;
+--echo # Con default
+SET DEBUG_SYNC= 'now WAIT_FOR removing_partitions';
+SET DEBUG_SYNC= 'waiting_for_table SIGNAL waiting_for_alter';
+SET DEBUG_SYNC= 'rm_table_part2_before_binlog SIGNAL delete_done';
+DROP TABLE IF EXISTS t2;
+--echo # Con 1
+connection con1;
+--error ER_NO_SUCH_TABLE
+--reap
+SET DEBUG_SYNC= 'RESET';
+disconnect con1;
+connection default;
+--echo # Con default
+SET DEBUG_SYNC= 'RESET';
+
+--echo End of 5.1 tests
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index 451631ff373..099b663f5c2 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -1719,13 +1719,23 @@ void ha_partition::update_create_info(HA_CREATE_INFO *create_info)
 
 void ha_partition::change_table_ptr(TABLE *table_arg, TABLE_SHARE *share)
 {
-  handler **file_array= m_file;
+  handler **file_array;
   table= table_arg;
   table_share= share;
-  do
+  /*
+    m_file can be NULL when using an old cached table in DROP TABLE, when the
+    table just has REMOVED PARTITIONING, see Bug#42438
+  */
+  if (m_file)
   {
-    (*file_array)->change_table_ptr(table_arg, share);
-  } while (*(++file_array));
+    file_array= m_file;
+    DBUG_ASSERT(*file_array);
+    do
+    {
+      (*file_array)->change_table_ptr(table_arg, share);
+    } while (*(++file_array));
+  }
+
   if (m_added_file && m_added_file[0])
   {
     /* if in middle of a drop/rename etc */
@@ -5986,7 +5996,13 @@ void ha_partition::print_error(int error, myf errflag)
   if (error == HA_ERR_NO_PARTITION_FOUND)
     m_part_info->print_no_partition_found(table);
   else
-    m_file[m_last_part]->print_error(error, errflag);
+  {
+    /* In case m_file has not been initialized, like in bug#42438 */
+    if (m_file)
+      m_file[m_last_part]->print_error(error, errflag);
+    else
+      handler::print_error(error, errflag);
+  }
   DBUG_VOID_RETURN;
 }
 
@@ -5996,7 +6012,12 @@ bool ha_partition::get_error_message(int error, String *buf)
   DBUG_ENTER("ha_partition::get_error_message");
 
   /* Should probably look for my own errors first */
-  DBUG_RETURN(m_file[m_last_part]->get_error_message(error, buf));
+
+  /* In case m_file has not been initialized, like in bug#42438 */
+  if (m_file)
+    DBUG_RETURN(m_file[m_last_part]->get_error_message(error, buf));
+  DBUG_RETURN(handler::get_error_message(error, buf));
+
 }
 
 
diff --git a/sql/sql_base.cc b/sql/sql_base.cc
index cf8a0b32764..b13a067bfd7 100644
--- a/sql/sql_base.cc
+++ b/sql/sql_base.cc
@@ -2165,6 +2165,7 @@ void wait_for_condition(THD *thd, pthread_mutex_t *mutex, pthread_cond_t *cond)
   proc_info=thd->proc_info;
   thd_proc_info(thd, "Waiting for table");
   DBUG_ENTER("wait_for_condition");
+  DEBUG_SYNC(thd, "waiting_for_table");
   if (!thd->killed)
     (void) pthread_cond_wait(cond, mutex);
 
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 869ae42c98c..5097c0b4945 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -22,6 +22,9 @@
 #include "sp_head.h"
 #include "sql_trigger.h"
 #include "sql_show.h"
+#if defined(ENABLED_DEBUG_SYNC)
+#include "debug_sync.h"
+#endif
 
 #ifdef __WIN__
 #include <io.h>
@@ -1870,30 +1873,6 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
 
   pthread_mutex_lock(&LOCK_open);
 
-  /*
-    If we have the table in the definition cache, we don't have to check the
-    .frm file to find if the table is a normal table (not view) and what
-    engine to use.
-  */
-
-  for (table= tables; table; table= table->next_local)
-  {
-    TABLE_SHARE *share;
-    table->db_type= NULL;
-    if ((share= get_cached_table_share(table->db, table->table_name)))
-      table->db_type= share->db_type();
-
-    /* Disable drop of enabled log tables */
-    if (share && (share->table_category == TABLE_CATEGORY_PERFORMANCE) &&
-        check_if_log_table(table->db_length, table->db,
-                           table->table_name_length, table->table_name, 1))
-    {
-      my_error(ER_BAD_LOG_STATEMENT, MYF(0), "DROP");
-      pthread_mutex_unlock(&LOCK_open);
-      DBUG_RETURN(1);
-    }
-  }
-
   if (!drop_temporary && lock_table_names_exclusively(thd, tables))
   {
     pthread_mutex_unlock(&LOCK_open);
@@ -1904,7 +1883,7 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
   {
     char *db=table->db;
     handlerton *table_type;
-    enum legacy_db_type frm_db_type;
+    enum legacy_db_type frm_db_type= DB_TYPE_UNKNOWN;
 
     DBUG_PRINT("table", ("table_l: '%s'.'%s'  table: 0x%lx  s: 0x%lx",
                          table->db, table->table_name, (long) table->table,
@@ -1945,6 +1924,15 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
       error= 0;
     }
 
+    /* Disable drop of enabled log tables */
+    if (check_if_log_table(table->db_length, table->db,
+                           table->table_name_length, table->table_name, 1))
+    {
+      my_error(ER_BAD_LOG_STATEMENT, MYF(0), "DROP");
+      pthread_mutex_unlock(&LOCK_open);
+      DBUG_RETURN(1);
+    }
+
     /*
       If row-based replication is used and the table is not a
       temporary table, we add the table name to the drop statement
@@ -1969,7 +1957,6 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
       built_query.append("`,");
     }
 
-    table_type= table->db_type;
     if (!drop_temporary)
     {
       TABLE *locked_table;
@@ -1996,9 +1983,9 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
                                         table->internal_tmp_table ?
                                         FN_IS_TMP : 0);
     }
+    DEBUG_SYNC(thd, "rm_table_part2_before_delete_table");
     if (drop_temporary ||
-        ((table_type == NULL &&        
-         access(path, F_OK) &&
+        ((access(path, F_OK) &&
           ha_create_table_from_engine(thd, db, alias)) ||
          (!drop_view &&
           mysql_frm_type(thd, path, &frm_db_type) != FRMTYPE_TABLE)))
@@ -2014,15 +2001,25 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
     else
     {
       char *end;
-      if (table_type == NULL)
+      /*
+        Cannot use the db_type from the table, since that might have changed
+        while waiting for the exclusive name lock. We are under LOCK_open,
+        so reading from the frm-file is safe.
+      */
+      if (frm_db_type == DB_TYPE_UNKNOWN)
       {
-	mysql_frm_type(thd, path, &frm_db_type);
-        table_type= ha_resolve_by_legacy_type(thd, frm_db_type);
+        mysql_frm_type(thd, path, &frm_db_type);
+        DBUG_PRINT("info", ("frm_db_type %d from %s", frm_db_type, path));
       }
+      table_type= ha_resolve_by_legacy_type(thd, frm_db_type);
       // Remove extension for delete
       *(end= path + path_length - reg_ext_length)= '\0';
+      DBUG_PRINT("info", ("deleting table of type %d",
+                          (table_type ? table_type->db_type : 0)));
       error= ha_delete_table(thd, table_type, path, db, table->table_name,
                              !dont_log_query);
+
+      /* No error if non existent table and 'IF EXIST' clause or view */
       if ((error == ENOENT || error == HA_ERR_NO_SUCH_TABLE) && 
 	  (if_exists || table_type == NULL))
       {
@@ -2062,6 +2059,7 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
     on the table name.
   */
   pthread_mutex_unlock(&LOCK_open);
+  DEBUG_SYNC(thd, "rm_table_part2_before_binlog");
   thd->thread_specific_used|= tmp_table_deleted;
   error= 0;
   if (wrong_tables.length())
@@ -7097,6 +7095,7 @@ view_err:
   else
     create_info->data_file_name=create_info->index_file_name=0;
 
+  DEBUG_SYNC(thd, "alter_table_before_create_table_no_lock");
   /*
     Create a table with a temporary name.
     With create_info->frm_only == 1 this creates a .frm file only.
@@ -7296,6 +7295,7 @@ view_err:
     intern_close_table(new_table);
     my_free(new_table,MYF(0));
   }
+  DEBUG_SYNC(thd, "alter_table_before_rename_result_table");
   VOID(pthread_mutex_lock(&LOCK_open));
   if (error)
   {
@@ -7438,6 +7438,7 @@ view_err:
   thd_proc_info(thd, "end");
 
   DBUG_EXECUTE_IF("sleep_alter_before_main_binlog", my_sleep(6000000););
+  DEBUG_SYNC(thd, "alter_table_before_main_binlog");
 
   ha_binlog_log_query(thd, create_info->db_type, LOGCOM_ALTER_TABLE,
                       thd->query(), thd->query_length(),