Backport of revno: 2617.68.37

Bug #46654 False deadlock on concurrent DML/DDL with partitions, 
           inconsistent behavior

The problem was that if one connection is running a multi-statement 
transaction which involves a single partitioned table, and another 
connection attempts to alter the table, the first connection gets 
ER_LOCK_DEADLOCK and cannot proceed anymore, even when the ALTER TABLE 
statement in another connection has timed out or failed.

The reason for this was that the prepare phase for ALTER TABLE for 
partitioned tables removed all instances of the table from the table 
definition cache before it started waiting on the lock. The transaction 
running in the first connection would notice this and report ER_LOCK_DEADLOCK. 

This patch changes the prep_alter_part_table() ALTER TABLE code so that 
tdc_remove_table() is no longer called. Instead, only the TABLE instance
changed by prep_alter_part_table() is marked as needing reopen.

The patch also removes an unnecessary call to tdc_remove_table() from 
mysql_unpack_partition() as the changed TABLE object is destroyed by the 
caller at a later point.

Test case added in partition_sync.test.

mysql-test/r/partition_sync.result

@@ -1,2 +1,34 @@
 # Disabled until Bug#46654 False deadlock on concurrent DML/DDL 
 # with partitions, inconsistent behavior is backported
+#
+# Bug #46654 False deadlock on concurrent DML/DDL 
+#            with partitions, inconsistent behavior
+#
+DROP TABLE IF EXISTS tbl_with_partitions;
+CREATE TABLE tbl_with_partitions ( i INT ) 
+PARTITION BY HASH(i);
+INSERT INTO tbl_with_partitions VALUES (1);
+# Connection 3
+LOCK TABLE tbl_with_partitions READ;
+# Connection 1
+# Access table with disabled autocommit
+SET AUTOCOMMIT = 0;
+SELECT * FROM tbl_with_partitions;
+i
+1
+# Connection 2
+# Alter table, abort after prepare
+set session debug="+d,abort_copy_table";
+ALTER TABLE tbl_with_partitions ADD COLUMN f INT;
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+# Connection 1
+# Try accessing the table after Alter aborted.
+# This used to give ER_LOCK_DEADLOCK.
+SELECT * FROM tbl_with_partitions;
+i
+1
+# Connection 3
+UNLOCK TABLES;
+# Connection 1
+# Cleanup
+DROP TABLE tbl_with_partitions;

mysql-test/t/partition_sync.test

@@ -39,6 +39,57 @@
 #DROP TABLE t1;
 
 
+--echo #
+--echo # Bug #46654 False deadlock on concurrent DML/DDL 
+--echo #            with partitions, inconsistent behavior
+--echo #
+
+--disable_warnings
+DROP TABLE IF EXISTS tbl_with_partitions;
+--enable_warnings
+
+CREATE TABLE tbl_with_partitions ( i INT ) 
+	PARTITION BY HASH(i);
+INSERT INTO tbl_with_partitions VALUES (1);
+
+connect(con2,localhost,root);
+connect(con3,localhost,root);
+
+--echo # Connection 3
+connection con3;
+LOCK TABLE tbl_with_partitions READ;
+
+--echo # Connection 1
+--echo # Access table with disabled autocommit
+connection default;
+SET AUTOCOMMIT = 0;
+SELECT * FROM tbl_with_partitions;
+
+--echo # Connection 2
+--echo # Alter table, abort after prepare
+connection con2;
+set session debug="+d,abort_copy_table";
+--error ER_LOCK_WAIT_TIMEOUT
+ALTER TABLE tbl_with_partitions ADD COLUMN f INT;
+
+--echo # Connection 1
+--echo # Try accessing the table after Alter aborted.
+--echo # This used to give ER_LOCK_DEADLOCK.
+connection default;
+SELECT * FROM tbl_with_partitions;
+
+--echo # Connection 3
+connection con3;
+UNLOCK TABLES;
+
+--echo # Connection 1
+--echo # Cleanup
+connection default;
+disconnect con2;
+disconnect con3;
+DROP TABLE tbl_with_partitions;
+
+
 # Check that all connections opened by test cases in this file are really
 # gone so execution of other tests won't be affected by their presence.
 --source include/wait_until_count_sessions.inc

sql/sql_base.cc

@@ -1530,8 +1530,8 @@ bool close_thread_table(THD *thd, TABLE **table_ptr)
   *table_ptr=table->next;
 
   table->mdl_ticket= NULL;
-  if (table->needs_reopen() ||
-      thd->version != refresh_version || !table->db_stat ||
+  if (table->s->needs_reopen() ||
+      thd->version != refresh_version || table->needs_reopen() ||
       table_def_shutdown_in_progress)
   {
     free_cache_entry(table);
@@ -8186,13 +8186,13 @@ bool mysql_notify_thread_having_shared_lock(THD *thd, THD *in_use)
        thd_table= thd_table->next)
   {
     /*
-      Check for TABLE::db_stat is needed since in some places we call
+      Check for TABLE::needs_reopen() is needed since in some places we call
       handler::close() for table instance (and set TABLE::db_stat to 0)
       and do not remove such instances from the THD::open_tables
       for some time, during which other thread can see those instances
       (e.g. see partitioning code).
     */
-    if (thd_table->db_stat)
+    if (!thd_table->needs_reopen())
       signalled|= mysql_lock_abort_for_thread(thd, thd_table);
   }
   pthread_mutex_unlock(&LOCK_open);

sql/sql_handler.cc

@@ -820,7 +820,7 @@ void mysql_ha_flush(THD *thd)
     if (hash_tables->table &&
         (hash_tables->table->mdl_ticket &&
          hash_tables->table->mdl_ticket->has_pending_conflicting_lock() ||
-         hash_tables->table->needs_reopen()))
+         hash_tables->table->s->needs_reopen()))
       mysql_ha_close_table(thd, hash_tables);
   }
 

sql/sql_insert.cc

@@ -2657,7 +2657,7 @@ bool Delayed_insert::handle_inserts(void)
 
   thd_proc_info(&thd, "insert");
   max_rows= delayed_insert_limit;
-  if (thd.killed || table->needs_reopen())
+  if (thd.killed || table->s->needs_reopen())
   {
     thd.killed= THD::KILL_CONNECTION;
     max_rows= ULONG_MAX;                     // Do as much as possible

sql/sql_partition.cc

@@ -4183,15 +4183,13 @@ bool mysql_unpack_partition(THD *thd,
       We need to free any memory objects allocated on item_free_list
       by the parser since we are keeping the old info from the first
       parser call in CREATE TABLE.
-      We'll ensure that this object isn't put into table cache also
-      just to ensure we don't get into strange situations with the
-      item objects.
+
+      This table object can not be used any more. However, since
+      this is CREATE TABLE, we know that it will be destroyed by the
+      caller, and rely on that.
     */
     thd->free_items();
     part_info= thd->work_part_info;
-    tdc_remove_table(thd, TDC_RT_REMOVE_UNUSED,
-                     table->s->db.str,
-                     table->s->table_name.str);
     *work_part_info_used= true;
   }
   table->part_info= part_info;
@@ -4484,17 +4482,11 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info,
 
   /*
     We are going to manipulate the partition info on the table object
-    so we need to ensure that the table instances cached and all other
-    instances are properly closed.
+    so we need to ensure that the table instance is removed from the
+    table cache.
   */
   if (table->part_info)
-  {
-    pthread_mutex_lock(&LOCK_open);
-    tdc_remove_table(thd, TDC_RT_REMOVE_UNUSED,
-                     table->s->db.str,
-                     table->s->table_name.str);
-    pthread_mutex_unlock(&LOCK_open);
-  }
+    table->m_needs_reopen= TRUE;
 
   thd->work_part_info= thd->lex->part_info;
   if (thd->work_part_info &&

sql/sql_table.cc

@@ -7049,6 +7049,10 @@ view_err:
     new_table->timestamp_field_type= TIMESTAMP_NO_AUTO_SET;
     new_table->next_number_field=new_table->found_next_number_field;
     thd_proc_info(thd, "copy to tmp table");
+    DBUG_EXECUTE_IF("abort_copy_table", {
+        my_error(ER_LOCK_WAIT_TIMEOUT, MYF(0));
+        goto err_new_table_cleanup;
+      });
     error= copy_data_between_tables(table, new_table,
                                     alter_info->create_list, ignore,
                                     order_num, order, &copied, &deleted,

sql/table.h

@@ -294,6 +294,8 @@ TABLE_CATEGORY get_table_category(const LEX_STRING *db,
 
 struct TABLE_share;
 
+extern ulong refresh_version;
+
 /*
   This structure is shared between different table objects. There is one
   instance of table share per one table in the database.
@@ -503,6 +505,14 @@ struct TABLE_SHARE
     return table_map_id;
   }
 
+
+  /*
+    Must all TABLEs be reopened?
+  */
+  inline bool needs_reopen()
+  {
+    return version != refresh_version;
+  }
   /**
     Convert unrelated members of TABLE_SHARE to one enum
     representing its type.
@@ -605,8 +615,6 @@ struct TABLE_SHARE
 };
 
 
-extern ulong refresh_version;
-
 /* Information for one open table */
 enum index_hint_type
 {
@@ -804,6 +812,7 @@ public:
   my_bool insert_or_update;             /* Can be used by the handler */
   my_bool alias_name_used;		/* true if table_name is alias */
   my_bool get_fields_in_item_tree;      /* Signal to fix_field */
+  my_bool m_needs_reopen;
 
   REGINFO reginfo;			/* field connections */
   MEM_ROOT mem_root;
@@ -853,7 +862,7 @@ public:
     Is this instance of the table should be reopen?
   */
   inline bool needs_reopen()
-  { return s->version != refresh_version; }
+  { return !db_stat || m_needs_reopen; }
 };