#At file:///Users/mattiasj/clones/bzrroot/b42438-51-bt/ based on revid:bar@stripped
3238 Mattias Jonsson 2010-02-01
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).
@ mysql-test/r/partition_debug_sync.result
Bug#42438: Crash ha_partition::change_table_ptr
New result file using DEBUG_SYNC for deterministic results.
@ mysql-test/t/partition_debug_sync.test
Bug#42438: Crash ha_partition::change_table_ptr
New test file using DEBUG_SYNC for deterministic results.
@ sql/ha_partition.cc
Bug#42438: Crash ha_partition::change_table_ptr
allow some handler calls, used by error handling, even when
no partitions are setup. Fallback to default handling if possible.
@ sql/sql_base.cc
Bug#42438: Crash ha_partition::change_table_ptr
Added DEBUG_SYNC point for deterministic test cases.
@ sql/sql_table.cc
Bug#42438: Crash ha_partition::change_table_ptr
Always use the table type written in the .frm-file
(i.e. the current table type) when deleting a table.
Moved the check for log-table to not depend of the cache.
Added DEBUG_SYNC points for deterministic test cases.
added:
mysql-test/r/partition_debug_sync.result
mysql-test/t/partition_debug_sync.test
modified:
sql/ha_partition.cc
sql/sql_base.cc
sql/sql_table.cc
=== added file 'mysql-test/r/partition_debug_sync.result'
--- a/mysql-test/r/partition_debug_sync.result 1970-01-01 00:00:00 +0000
+++ b/mysql-test/r/partition_debug_sync.result 2010-02-01 15:07:00 +0000
@@ -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
=== added file 'mysql-test/t/partition_debug_sync.test'
--- a/mysql-test/t/partition_debug_sync.test 1970-01-01 00:00:00 +0000
+++ b/mysql-test/t/partition_debug_sync.test 2010-02-01 15:07:00 +0000
@@ -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
=== modified file 'sql/ha_partition.cc'
--- a/sql/ha_partition.cc 2009-10-09 07:56:07 +0000
+++ b/sql/ha_partition.cc 2010-02-01 15:07:00 +0000
@@ -1719,13 +1719,23 @@ void ha_partition::update_create_info(HA
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
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
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));
+
}
=== modified file 'sql/sql_base.cc'
--- a/sql/sql_base.cc 2009-11-20 20:56:43 +0000
+++ b/sql/sql_base.cc 2010-02-01 15:07:00 +0000
@@ -2165,6 +2165,7 @@ void wait_for_condition(THD *thd, pthrea
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);
=== modified file 'sql/sql_table.cc'
--- a/sql/sql_table.cc 2009-11-27 13:34:39 +0000
+++ b/sql/sql_table.cc 2010-02-01 15:07:00 +0000
@@ -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
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
{
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
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
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
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
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
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(),
Attachment: [text/bzr-bundle]
