Fix for bug #25044 "ALTER TABLE ... ENABLE KEYS acquires global 'opening

tables' lock."

Execution of ALTER TABLE ... ENABLE KEYS on a table (which can take rather
long time) prevented concurrent execution of all statements using tables.

The problem was caused by the fact that we were holding LOCK_open mutex
during whole duration of this statement and particularly during call
to handler::enable_indexes(). This behavior was introduced as part of the
fix for bug 14262 "SP: DROP PROCEDURE|VIEW (maybe more) write to binlog
too late (race cond)"

The patch simply restores old behavior. Note that we can safely do this as
this operation takes exclusive lock (similar to name-lock) which blocks both
DML and DDL on the table being altered.

It also introduces mysql-test/include/wait_show_pattern.inc helper script
which is used to make test-case for this bug robust enough.

mysql-test/include/wait_show_pattern.inc

+# include/wait_show_pattern.inc
+#
+# SUMMARY
+#
+#   Waits until output produced by SHOW statement which particular type is
+#   specified as parameter matches certain pattern or maximum time reached.
+#
+# NOTES
+#
+#   Only the first row produced by the parameter statement is checked.
+#
+# USAGE
+#
+#   let $show_type= <Tail of SHOW statement>;
+#   let $show_pattern= 'Pattern to be used for LIKE matching';
+#   --source wait_show_pattern.inc
+#
+# EXAMPLES
+# 
+#   alter_table-big.test, wait_slave_status.inc
+#
+# SEE ALSO
+#
+#   wait_slave_status.inc, wait_condition.inc (>=5.1)
+#
+###############################################################################
+
+--disable_query_log
+
+# We accept to wait maximum 30 seconds (0.2 sec/loop).
+let $wait_counter= 150;
+while ($wait_counter)
+{
+  let $result= `SHOW $show_type`;
+  let $success= `SELECT '$result' LIKE $show_pattern`;
+  if ($success)
+  {
+    let $wait_counter= 0;
+  }
+  if (!$success)
+  {
+    real_sleep 0.2;
+    dec $wait_counter;
+  }
+}
+if (!$success)
+{
+  echo Timeout in wait_show_pattern.inc \$show_type= $show_type \$show_pattern= $show_pattern (\$result= '$result');
+}
+
+--enable_query_log

mysql-test/include/wait_slave_status.inc

@@ -104,50 +104,21 @@
 eval SELECT "let \$result_pattern= $result_pattern ;" AS "";
 SELECT '--source include/wait_slave_status.inc' AS "";
 
-# We accept to wait maximum 30 seconds (0.2 sec/loop).
-let $max_wait= 150;
-while ($max_wait)
-{
-    let $my_val= `SHOW SLAVE STATUS`;
-    # Now we have the first record of the SHOW result set as one fat string
-    # within the variable  $my_val.
-
-    eval SET @my_val = '$my_val';
-    # DEBUG eval SELECT @my_val AS "response to SHOW SLAVE STATUS";
+let $show_type= SLAVE STATUS;
+let $show_pattern= $result_pattern;
+--enable_query_log
 
-    eval SELECT @my_val LIKE $result_pattern INTO @success;
-    # @success is     '1' if we have a match
-    #                 '0' if we have no match
-    # DEBUG SELECT @success;
+--source include/wait_show_pattern.inc
 
-    let $success= `SELECT @success`;
-    let $no_success= `SELECT @success = 0`;
-    if ($success)
-    {
-       # We reached the expected result and want to jump out of the loop
-       # without unneeded sleeps.
-       # Attention: Do not set $max_wait to 0, because "while" with negative value
-       #            does not work.
-       let $max_wait= 1;
-    }
-    if ($no_success)
-    {
-       # We did not reach the expected result and will have to sleep again
-       # or jump out of the loop, when max_wait is exhausted.
-       real_sleep 0.2;
-    }
-    dec $max_wait;
-}
---enable_query_log
-if ($no_success)
+if (!$success)
 {
 let $message= ! Attention: Timeout in wait_slave_status.inc.
               |       Possible reasons with decreasing probability:
-              |       - The LIKE pattern ($result_pattern) is wrong, because the
+              |       - The LIKE pattern is wrong, because the
               |         testcase was altered or the layout of the
               |         SHOW SLAVE STATUS result set changed.
               |       - There is a new bug within the replication.
-              |       - We met an extreme testing environment and $max_wait is
+              |       - We met an extreme testing environment and timeout is
               |         too small.;
 --source include/show_msg80.inc
 --echo DEBUG INFO START (wait_slave_status.inc):

mysql-test/r/alter_table-big.result

+drop table if exists t1, t2;
+create table t1 (n1 int, n2 int, n3 int,
+key (n1, n2, n3),
+key (n2, n3, n1),
+key (n3, n1, n2));
+create table t2 (i int);
+alter table t1 disable keys;
+reset master;
+alter table t1 enable keys;;
+insert into t2 values (1);
+insert into t1 values (1, 1, 1);
+show binlog events in 'master-bin.000001' from 98;
+Log_name	Pos	Event_type	Server_id	End_log_pos	Info
+master-bin.000001	#	Query	1	#	use `test`; insert into t2 values (1)
+master-bin.000001	#	Query	1	#	use `test`; alter table t1 enable keys
+master-bin.000001	#	Query	1	#	use `test`; insert into t1 values (1, 1, 1)
+drop tables t1, t2;
+End of 5.0 tests

mysql-test/t/alter_table-big.test

+# In order to be more or less robust test for bug#25044 has to take
+# significant time (e.g. about 9 seconds on my (Dmitri's) computer)
+# so we probably want execute it only in --big-test mode.
+# Also in 5.1 this test will require statement-based binlog.
+--source include/big_test.inc
+
+
+#
+# Test for bug #25044 "ALTER TABLE ... ENABLE KEYS acquires global
+# 'opening tables' lock".
+#
+# ALTER TABLE ... ENABLE KEYS should not acquire LOCK_open mutex for
+# the whole its duration as it prevents other queries from execution.
+--disable_warnings
+drop table if exists t1, t2;
+--enable_warnings
+connect (addconroot, localhost, root,,);
+connection default;
+create table t1 (n1 int, n2 int, n3 int,
+                key (n1, n2, n3),
+                key (n2, n3, n1),
+                key (n3, n1, n2));
+create table t2 (i int);
+
+# Populating 't1' table with keys disabled, so ALTER TABLE .. ENABLE KEYS
+# will run for some time
+alter table t1 disable keys;
+--disable_query_log
+insert into t1 values (RAND()*1000,RAND()*1000,RAND()*1000);
+let $1=19;
+while ($1)
+{
+ eval insert into t1 select RAND()*1000,RAND()*1000,RAND()*1000 from t1;
+ dec $1;
+}
+--enable_query_log
+
+# Later we use binlog to check the order in which statements are
+# executed so let us reset it first.
+reset master;
+--send alter table t1 enable keys;
+connection addconroot;
+let $show_type= PROCESSLIST;
+let $show_pattern= '%Repair by sorting%alter table t1 enable keys%';
+--source include/wait_show_pattern.inc
+# This statement should not be blocked by in-flight ALTER and therefore
+# should be executed and written to binlog before ALTER TABLE ... ENABLE KEYS
+# finishes.
+insert into t2 values (1);
+# And this should wait until the end of ALTER TABLE ... ENABLE KEYS.
+insert into t1 values (1, 1, 1);
+connection default;
+--reap
+# Check that statements were executed/binlogged in correct order.
+--replace_column 2 # 5 #
+show binlog events in 'master-bin.000001' from 98;
+
+# Clean up
+drop tables t1, t2;
+
+
+--echo End of 5.0 tests

sql/sql_table.cc

@@ -3146,19 +3146,30 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
   if (!(alter_info->flags & ~(ALTER_RENAME | ALTER_KEYS_ONOFF)) &&
       !table->s->tmp_table) // no need to touch frm
   {
-    VOID(pthread_mutex_lock(&LOCK_open));
-
     switch (alter_info->keys_onoff) {
     case LEAVE_AS_IS:
       error= 0;
       break;
     case ENABLE:
+      /*
+        wait_while_table_is_used() ensures that table being altered is
+        opened only by this thread and that TABLE::TABLE_SHARE::version
+        of TABLE object corresponding to this table is 0.
+        The latter guarantees that no DML statement will open this table
+        until ALTER TABLE finishes (i.e. until close_thread_tables())
+        while the fact that the table is still open gives us protection
+        from concurrent DDL statements.
+      */
+      VOID(pthread_mutex_lock(&LOCK_open));
       wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN);
+      VOID(pthread_mutex_unlock(&LOCK_open));
       error= table->file->enable_indexes(HA_KEY_SWITCH_NONUNIQ_SAVE);
       /* COND_refresh will be signaled in close_thread_tables() */
       break;
     case DISABLE:
+      VOID(pthread_mutex_lock(&LOCK_open));
       wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN);
+      VOID(pthread_mutex_unlock(&LOCK_open));
       error=table->file->disable_indexes(HA_KEY_SWITCH_NONUNIQ_SAVE);
       /* COND_refresh will be signaled in close_thread_tables() */
       break;
@@ -3171,6 +3182,16 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
       error= 0;
     }
 
+    VOID(pthread_mutex_lock(&LOCK_open));
+    /*
+      Unlike to the above case close_cached_table() below will remove ALL
+      instances of TABLE from table cache (it will also remove table lock
+      held by this thread). So to make actual table renaming and writing
+      to binlog atomic we have to put them into the same critical section
+      protected by LOCK_open mutex. This also removes gap for races between
+      access() and mysql_rename_table() calls.
+    */
+
     if (!error && (new_name != table_name || new_db != db))
     {
       thd->proc_info="rename";