Merge lp:~laurynas-biveinis/percona-server/bug951588-5.1 into lp:percona-server/5.1

[Laurynas Biveinis]

Fix bug 951588 (Querying I_S.GLOBAL_TEMPORARY_TABLES or
TEMPORARY_TABLES crashes threads working with temp tables)

The issue is caused by fill_global_temporary_tables() in sql_show.cc
enumerating existing temp table handler objects that are possibly in
use by other threads. Then for each object the handler::info() method
is called in store_temporary_table_record() that proceeds to overwrite
current_thd, prebuilt->trx and possibly other fields of any operation
in progress for that temp table handler.

The fix is to not invoke handler::info() method on the existing handler
objects in order not to interfere with their state. Instead we use
handler::clone() method first to get an independent handler pointing
to the same table and use it to invoke the handler::info() method and
read the stats.

Also simplify the code to get rid of "handle" local variable that
aliases already existing "file". Fix the type of "engineType" to
const char* and remove unnecessary cast.

For testing add new debug sync point "start_ha_write_row" at the start
of handler::ha_write_row.

Add new test percona_bug_951988 that crashes without the fix and
passes with it.

Alexey's review comments have been addressed.

Jenkins: http://jenkins.percona.com/job/percona-server-5.1-param/326/
Issue #23438.

[Alexey Kopytov (akopytov)]

Laurynas,

This looks good from a functional perspective. But the code in store_temporary_table_record() is a bit messy, and the patch even adds a bit to that mess instead of cleaning it up.

There are 2 copies of the temp. table handler:

- 'handle' which was only used to get the storage engine type, and now, to clone file_handle_2
- 'file' which was used to get the actual stat values, and is now unused.

What about making 'file' a pointer to the cloned handler object. That would result in a bit cleaner code and less lines to touch?

review: Needs Fixing

[Alexey Kopytov (akopytov)]

(and nothing prevents us from using the same 'file' pointer to the cloned handler object to get the storage engine name, and get rid of 'handle' completely).

[Alexey Kopytov (akopytov)]

review: Approve

=== added file 'Percona-Server/mysql-test/suite/innodb_plugin/r/percona_bug_951588.result'
--- Percona-Server/mysql-test/suite/innodb_plugin/r/percona_bug_951588.result	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/innodb_plugin/r/percona_bug_951588.result	2012-06-08 03:54:21 +0000
@@ -0,0 +1,12 @@
+DROP TABLE IF EXISTS t1;
+CREATE TEMPORARY TABLE t1 (a INT) ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1), (2), (3);
+SET DEBUG_SYNC= 'start_ha_write_row SIGNAL write_in_progress WAIT_FOR i_s_completed';
+ALTER TABLE t1 ADD COLUMN b VARCHAR(10);
+SET DEBUG_SYNC= 'now WAIT_FOR write_in_progress';
+SELECT COUNT(*) FROM INFORMATION_SCHEMA.GLOBAL_TEMPORARY_TABLES;
+COUNT(*)
+2
+SET DEBUG_SYNC= 'now SIGNAL i_s_completed';
+DROP TABLE t1;
+SET DEBUG_SYNC='reset';

=== added file 'Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug_951588.test'
--- Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug_951588.test	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/innodb_plugin/t/percona_bug_951588.test	2012-06-08 03:54:21 +0000
@@ -0,0 +1,29 @@
+# Test for bug 951588 (Querying I_S.TEMPORARY_TABLES crashes parallel threads working on temp tables)
+
+--source include/have_innodb_plugin.inc
+--source include/have_debug_sync.inc
+
+--disable_warnings
+DROP TABLE IF EXISTS t1;
+--enable_warnings
+
+CREATE TEMPORARY TABLE t1 (a INT) ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1), (2), (3);
+
+SET DEBUG_SYNC= 'start_ha_write_row SIGNAL write_in_progress WAIT_FOR i_s_completed';
+send ALTER TABLE t1 ADD COLUMN b VARCHAR(10);
+
+connect (conn2,localhost,root,,);
+connection conn2;
+
+SET DEBUG_SYNC= 'now WAIT_FOR write_in_progress';
+SELECT COUNT(*) FROM INFORMATION_SCHEMA.GLOBAL_TEMPORARY_TABLES;
+SET DEBUG_SYNC= 'now SIGNAL i_s_completed';
+
+disconnect conn2;
+connection default;
+reap;
+
+DROP TABLE t1;
+
+SET DEBUG_SYNC='reset';

=== modified file 'Percona-Server/sql/handler.cc'
--- Percona-Server/sql/handler.cc	2012-05-09 04:14:12 +0000
+++ Percona-Server/sql/handler.cc	2012-06-08 03:54:21 +0000
@@ -27,6 +27,7 @@
 #include "rpl_filter.h"
 #include <myisampack.h>
 #include <errno.h>
+#include "debug_sync.h"         // DEBUG_SYNC
 
 #ifdef WITH_PARTITION_STORAGE_ENGINE
 #include "ha_partition.h"
@@ -4795,6 +4796,7 @@
   int error;
   Log_func *log_func= Write_rows_log_event::binlog_row_logging_function;
   DBUG_ENTER("handler::ha_write_row");
+  DEBUG_SYNC(ha_thd(), "start_ha_write_row");
 
   mark_trx_read_write();
 

=== modified file 'Percona-Server/sql/sql_show.cc'
--- Percona-Server/sql/sql_show.cc	2012-05-09 04:14:12 +0000
+++ Percona-Server/sql/sql_show.cc	2012-06-08 03:54:21 +0000
@@ -3694,8 +3694,9 @@
     DBUG_RETURN(schema_table_store_record(thd, table));
 
   //engine
-  handler *handle= tmp_table->file;
-  char *engineType = (char *)(handle ? handle->table_type() : "UNKNOWN");
+  handler *file= tmp_table->file;
+  // Assume that invoking handler::table_type() on a shared handler is safe
+  const char *engineType = (file) ? file->table_type() : "UNKNOWN";
   table->field[3]->store(engineType, strlen(engineType), cs);
 
   //name
@@ -3706,12 +3707,15 @@
   }
 
   // file stats
-  handler *file= tmp_table->file;
-
   if (file) {
 
     MYSQL_TIME time;
 
+    /* We have only one handler object for a temp table globally and it might
+    be in use by other thread.  Do not trash it by invoking handler methods on
+    it but rather clone it. */
+    file = file->clone(tmp_table->s->normalized_path.str, thd->mem_root);
+
     /**
         TODO: InnoDB stat(file) checks file on short names within data dictionary
         rather than using full path, because of that, temp files created in
@@ -3741,6 +3745,8 @@
       table->field[10]->store_time(&time, MYSQL_TIMESTAMP_DATETIME);
       table->field[10]->set_notnull();
     }
+
+    file->close();
   }
 
   DBUG_RETURN(schema_table_store_record(thd, table));