Bug #42230 during add index, cannot do queries on storage engines

           that implement add_index

The problem was that ALTER TABLE blocked reads on an InnoDB table
while adding a secondary index, even if this was not needed. It is
only needed for the final step where the .frm file is updated.

The reason queries were blocked, was that ALTER TABLE upgraded the
metadata lock from MDL_SHARED_NO_WRITE (which blocks writes) to
MDL_EXCLUSIVE (which blocks all accesses) before index creation.

The way the server handles index creation, is that storage engines
publish their capabilities to the server and the server determines
which of the following three ways this can be handled: 1) build a
new version of the table; 2) change the existing table but with
exclusive metadata lock; 3) change the existing table but without
metadata lock upgrade.

For InnoDB and secondary index creation, option 3) should have been
selected. However this failed for two reasons. First, InnoDB did
not publish this capability properly.

Second, the ALTER TABLE code failed to made proper use of the
information supplied by the storage engine. A variable
need_lock_for_indexes was set accordingly, but was not later used.
This patch fixes this problem by only doing metadata lock upgrade
before index creation/deletion if this variable has been set.

This patch also changes some of the related terminology used 
in the code. Specifically the use of "fast" and "online" with
respect to ALTER TABLE. "Fast" was used to indicate that an
ALTER TABLE operation could be done without involving a
temporary table. "Fast" has been renamed "in-place" to more
accurately describe the behavior.

"Online" meant that the operation could be done without taking
a table lock. However, in the current implementation writes
are always prohibited during ALTER TABLE and an exclusive
metadata lock is held while updating the .frm, so ALTER TABLE
is not completely online. This patch replaces "online" with 
"in-place", with additional comments indicating if concurrent
reads are allowed during index creation/deletion or not.

An important part of this update of terminology is renaming
of the handler flags used by handlers to indicate if index
creation/deletion can be done in-place and if concurrent reads
are allowed. For example, the HA_ONLINE_ADD_INDEX_NO_WRITES
flag has been renamed to HA_INPLACE_ADD_INDEX_NO_READ_WRITE,
while HA_ONLINE_ADD_INDEX is now HA_INPLACE_ADD_INDEX_NO_WRITE.
Note that this is a rename to clarify current behavior, the
flag values have not changed and no flags have been removed or
added.

Test case added to innodb_mysql_sync.test.

1 files changed, 47 insertions, 35 deletions

diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 064c2b36e17..2855bb0ec4a 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -1,4 +1,4 @@
-/* Copyright 2000-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
+/* Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -4745,7 +4745,7 @@ err:
  
   @details Checks if any index is being modified (present as both DROP INDEX 
     and ADD INDEX) in the current ALTER TABLE statement. Needed for disabling 
-    online ALTER TABLE.
+    in-place ALTER TABLE.
   
   @param table       The table being altered
   @param alter_info  The ALTER TABLE structure
@@ -4861,7 +4861,7 @@ mysql_compare_tables(TABLE *table,
     like to keep mysql_compare_tables() idempotent (not altering any
     of the arguments) we create a copy of alter_info here and
     pass it to mysql_prepare_create_table, then use the result
-    to evaluate possibility of fast ALTER TABLE, and then
+    to evaluate possibility of in-place ALTER TABLE, and then
     destroy the copy.
   */
   Alter_info tmp_alter_info(*alter_info, thd->mem_root);
@@ -4902,9 +4902,9 @@ mysql_compare_tables(TABLE *table,
 
     There was a bug prior to mysql-4.0.25. Number of null fields was
     calculated incorrectly. As a result frm and data files gets out of
-    sync after fast alter table. There is no way to determine by which
+    sync after in-place alter table. There is no way to determine by which
     mysql version (in 4.0 and 4.1 branches) table was created, thus we
-    disable fast alter table for all tables created by mysql versions
+    disable in-place alter table for all tables created by mysql versions
     prior to 5.0 branch.
     See BUG#6236.
   */
@@ -4927,7 +4927,7 @@ mysql_compare_tables(TABLE *table,
   }
 
   /*
-    Use transformed info to evaluate possibility of fast ALTER TABLE
+    Use transformed info to evaluate possibility of in-place ALTER TABLE
     but use the preserved field to persist modifications.
   */
   new_field_it.init(alter_info->create_list);
@@ -5207,7 +5207,7 @@ blob_length_by_type(enum_field_types type)
   semantic checks.
 
   This function is invoked when we know that we're going to
-  perform ALTER TABLE via a temporary table -- i.e. fast ALTER TABLE
+  perform ALTER TABLE via a temporary table -- i.e. in-place ALTER TABLE
   is not possible, perhaps because the ALTER statement contains
   instructions that require change in table data, not only in
   table definition or indexes.
@@ -6102,7 +6102,7 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
   }
 
   /*
-    If there are index changes only, try to do them online. "Index
+    If there are index changes only, try to do them in-place. "Index
     changes only" means also that the handler for the table does not
     change. The table is open and locked. The handler can be accessed.
   */
@@ -6110,8 +6110,8 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
   {
     int   pk_changed= 0;
     ulong alter_flags= 0;
-    ulong needed_online_flags= 0;
-    ulong needed_fast_flags= 0;
+    ulong needed_inplace_with_read_flags= 0;
+    ulong needed_inplace_flags= 0;
     KEY   *key;
     uint  *idx_p;
     uint  *idx_end_p;
@@ -6135,8 +6135,8 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
         {
           DBUG_PRINT("info", ("Dropping primary key"));
           /* Primary key. */
-          needed_online_flags|=  HA_ONLINE_DROP_PK_INDEX;
-          needed_fast_flags|= HA_ONLINE_DROP_PK_INDEX_NO_WRITES;
+          needed_inplace_with_read_flags|= HA_INPLACE_DROP_PK_INDEX_NO_WRITE;
+          needed_inplace_flags|= HA_INPLACE_DROP_PK_INDEX_NO_READ_WRITE;
           pk_changed++;
           candidate_key_count--;
         }
@@ -6146,8 +6146,9 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
           bool is_candidate_key= true;
 
           /* Non-primary unique key. */
-          needed_online_flags|=  HA_ONLINE_DROP_UNIQUE_INDEX;
-          needed_fast_flags|= HA_ONLINE_DROP_UNIQUE_INDEX_NO_WRITES;
+          needed_inplace_with_read_flags|=
+            HA_INPLACE_DROP_UNIQUE_INDEX_NO_WRITE;
+          needed_inplace_flags|= HA_INPLACE_DROP_UNIQUE_INDEX_NO_READ_WRITE;
 
           /*
             Check if all fields in key are declared
@@ -6166,12 +6167,13 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
       else
       {
         /* Non-unique key. */
-        needed_online_flags|=  HA_ONLINE_DROP_INDEX;
-        needed_fast_flags|= HA_ONLINE_DROP_INDEX_NO_WRITES;
+        needed_inplace_with_read_flags|= HA_INPLACE_DROP_INDEX_NO_WRITE;
+        needed_inplace_flags|= HA_INPLACE_DROP_INDEX_NO_READ_WRITE;
       }
     }
     no_pk= ((table->s->primary_key == MAX_KEY) ||
-            (needed_online_flags & HA_ONLINE_DROP_PK_INDEX));
+            (needed_inplace_with_read_flags &
+             HA_INPLACE_DROP_PK_INDEX_NO_WRITE));
     /* Check added indexes. */
     for (idx_p= index_add_buffer, idx_end_p= idx_p + index_add_count;
          idx_p < idx_end_p;
@@ -6209,57 +6211,59 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
         {
           DBUG_PRINT("info", ("Adding primary key"));
           /* Primary key. */
-          needed_online_flags|=  HA_ONLINE_ADD_PK_INDEX;
-          needed_fast_flags|= HA_ONLINE_ADD_PK_INDEX_NO_WRITES;
+          needed_inplace_with_read_flags|= HA_INPLACE_ADD_PK_INDEX_NO_WRITE;
+          needed_inplace_flags|= HA_INPLACE_ADD_PK_INDEX_NO_READ_WRITE;
           pk_changed++;
           no_pk= false;
         }
         else
         {
           /* Non-primary unique key. */
-          needed_online_flags|=  HA_ONLINE_ADD_UNIQUE_INDEX;
-          needed_fast_flags|= HA_ONLINE_ADD_UNIQUE_INDEX_NO_WRITES;
+          needed_inplace_with_read_flags|= HA_INPLACE_ADD_UNIQUE_INDEX_NO_WRITE;
+          needed_inplace_flags|= HA_INPLACE_ADD_UNIQUE_INDEX_NO_READ_WRITE;
         }
       }
       else
       {
         /* Non-unique key. */
-        needed_online_flags|=  HA_ONLINE_ADD_INDEX;
-        needed_fast_flags|= HA_ONLINE_ADD_INDEX_NO_WRITES;
+        needed_inplace_with_read_flags|= HA_INPLACE_ADD_INDEX_NO_WRITE;
+        needed_inplace_flags|= HA_INPLACE_ADD_INDEX_NO_READ_WRITE;
       }
     }
 
-    if ((candidate_key_count > 0) && 
-        (needed_online_flags & HA_ONLINE_DROP_PK_INDEX))
+    if ((candidate_key_count > 0) &&
+        (needed_inplace_with_read_flags & HA_INPLACE_DROP_PK_INDEX_NO_WRITE))
     {
       /*
         Dropped primary key when there is some other unique 
         not null key that should be converted to primary key
       */
-      needed_online_flags|=  HA_ONLINE_ADD_PK_INDEX;
-      needed_fast_flags|= HA_ONLINE_ADD_PK_INDEX_NO_WRITES;
+      needed_inplace_with_read_flags|= HA_INPLACE_ADD_PK_INDEX_NO_WRITE;
+      needed_inplace_flags|= HA_INPLACE_ADD_PK_INDEX_NO_READ_WRITE;
       pk_changed= 2;
     }
 
-    DBUG_PRINT("info", ("needed_online_flags: 0x%lx, needed_fast_flags: 0x%lx",
-                        needed_online_flags, needed_fast_flags));
+    DBUG_PRINT("info",
+          ("needed_inplace_with_read_flags: 0x%lx, needed_inplace_flags: 0x%lx",
+           needed_inplace_with_read_flags, needed_inplace_flags));
     /*
-      Online or fast add/drop index is possible only if
+      In-place add/drop index is possible only if
       the primary key is not added and dropped in the same statement.
       Otherwise we have to recreate the table.
       need_copy_table is no-zero at this place.
     */
     if ( pk_changed < 2 )
     {
-      if ((alter_flags & needed_online_flags) == needed_online_flags)
+      if ((alter_flags & needed_inplace_with_read_flags) ==
+          needed_inplace_with_read_flags)
       {
-        /* All required online flags are present. */
+        /* All required in-place flags to allow concurrent reads are present. */
         need_copy_table= ALTER_TABLE_METADATA_ONLY;
         need_lock_for_indexes= FALSE;
       }
-      else if ((alter_flags & needed_fast_flags) == needed_fast_flags)
+      else if ((alter_flags & needed_inplace_flags) == needed_inplace_flags)
       {
-        /* All required fast flags are present. */
+        /* All required in-place flags are present. */
         need_copy_table= ALTER_TABLE_METADATA_ONLY;
       }
     }
@@ -6413,10 +6417,18 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name,
   }
   else
   {
-    if (!table->s->tmp_table &&
+    /*
+      Ensure that we will upgrade the metadata lock if
+      handler::enable/disable_indexes() will be called.
+    */
+    if (alter_info->keys_onoff != LEAVE_AS_IS ||
+        table->file->indexes_are_disabled())
+      need_lock_for_indexes= true;
+    if (!table->s->tmp_table && need_lock_for_indexes &&
         wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN))
       goto err_new_table_cleanup;
     thd_proc_info(thd, "manage keys");
+    DEBUG_SYNC(thd, "alter_table_manage_keys");
     alter_table_manage_keys(table, table->file->indexes_are_disabled(),
                             alter_info->keys_onoff);
     error= trans_commit_stmt(thd);