bk commit into 5.0 tree (dlenev:1.2354) BUG#25044

[dlenev]

Below is the list of changes that have just been committed into a local
5.0 repository of dlenev. When dlenev does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-01-04 11:19:31+03:00, dlenev@stripped +3 -0
  Proposed 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.
  
  Question for reviewers is marked by QQ.

  mysql-test/r/alter_table-big.result@stripped, 2007-01-04 11:19:29+03:00,
dlenev@stripped +18 -0
    New BitKeeper file ``mysql-test/r/alter_table-big.result''

  mysql-test/r/alter_table-big.result@stripped, 2007-01-04 11:19:29+03:00,
dlenev@stripped +0 -0

  mysql-test/t/alter_table-big.test@stripped, 2007-01-04 11:19:29+03:00,
dlenev@stripped +68 -0
    New BitKeeper file ``mysql-test/t/alter_table-big.test''

  mysql-test/t/alter_table-big.test@stripped, 2007-01-04 11:19:29+03:00,
dlenev@stripped +0 -0

  sql/sql_table.cc@stripped, 2007-01-04 11:19:29+03:00, dlenev@stripped +22 -2
    mysql_alter_table():
      Changed ALTER TABLE ... ENABLE/DISABLE KEYS not to hold LOCK_open mutex
      during call to handler::enable_indexes() as the latter can take rather
      long time and therefore such ALTER would block execution of all other
      statements that use tables. We can safely do this as this operation takes 
      exclusive lock (similar to name-lock) on the table which is altered.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	dlenev
# Host:	mockturtle.local
# Root:	/home/dlenev/src/mysql-5.0-bg25044

--- 1.329/sql/sql_table.cc	2007-01-04 11:19:35 +03:00
+++ 1.330/sql/sql_table.cc	2007-01-04 11:19:35 +03:00
@@ -3146,19 +3146,30 @@
   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;
@@ -3170,6 +3181,15 @@
 			  table->alias);
       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.
+    */
 
     if (!error && (new_name != table_name || new_db != db))
     {
--- New file ---
+++ mysql-test/r/alter_table-big.result	07/01/04 11:19:29
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

--- New file ---
+++ mysql-test/t/alter_table-big.test	07/01/04 11:19:29
# 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.
#
# QQ: Altough I have tried to make this test robust enough it
#     surely will fail once computers will become fast enough.
#     So may be instead of relying on big table we should use
#     conditional sleep in debug mode approach ?
#     And/or may be we should add this test to some alternative
#     test-suite ?
#
--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;
--sleep 2
# 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