List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:November 21 2011 9:13am
Subject:bzr push into mysql-trunk-wl5534 branch (Dmitry.Lenev:3427 to 3428) WL#5534
View as plain text  
 3428 Dmitry Lenev	2011-11-21
      WL#5534 Online ALTER, Phase 1.
      
      Patch #51: Review changes.
      
      - Removed Alter_inplace_info::CHANGE_COLUMN flag.
      - Changed fill_alter_inplace_info() to properly
        handle key comparison in cases when some columns
        change their positions.
      - Updated result of innodb-index.test in which one
        query started to use pre-WL#5534 execution plan.

    modified:
      mysql-test/suite/innodb/r/innodb-index.result
      sql/handler.cc
      sql/handler.h
      sql/sql_table.cc
 3427 Jon Olav Hauglid	2011-11-16 [merge]
      Merge from mysql-trunk to mysql-trunk-wl5534
      No conflicts

    removed:
      mysql-test/extra/rpl_tests/rpl_parallel_benchmark_load.test
      mysql-test/suite/rpl/t/rpl_parallel_benchmark-master.opt
      mysql-test/suite/rpl/t/rpl_parallel_benchmark-slave.opt
      mysql-test/suite/rpl/t/rpl_parallel_benchmark.test
    added:
      mysql-test/suite/rpl/r/rpl_row_find_row_debug.result
      mysql-test/suite/rpl/t/rpl_row_find_row_debug.test
      mysql-test/suite/sys_vars/r/metadata_locks_cache_size_basic.result
      mysql-test/suite/sys_vars/t/metadata_locks_cache_size_basic-master.opt
      mysql-test/suite/sys_vars/t/metadata_locks_cache_size_basic.test
    modified:
      client/mysql_upgrade.c
      mysql-test/extra/rpl_tests/rpl_parallel_load.test
      mysql-test/r/mysql_upgrade.result
      mysql-test/r/mysqld--help-notwin.result
      mysql-test/r/mysqld--help-win.result
      mysql-test/t/mysql_upgrade.test
      sql/item_func.cc
      sql/log_event.cc
      sql/log_event.h
      sql/mdl.cc
      sql/mdl.h
      sql/rpl_rli.cc
      sql/rpl_rli.h
      sql/sql_class.h
      sql/sql_partition.cc
      sql/sql_plist.h
      sql/sys_vars.cc
=== modified file 'mysql-test/suite/innodb/r/innodb-index.result'
--- a/mysql-test/suite/innodb/r/innodb-index.result	2011-10-24 13:25:09 +0000
+++ b/mysql-test/suite/innodb/r/innodb-index.result	2011-11-21 09:13:00 +0000
@@ -951,7 +951,7 @@ Table	Op	Msg_type	Msg_text
 test.t1	check	status	OK
 explain select * from t1 where b like 'adfd%';
 id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
-1	SIMPLE	t1	range	b	b	769	NULL	11	Using where
+1	SIMPLE	t1	ALL	b	NULL	NULL	NULL	15	Using where
 drop table t1;
 set global innodb_file_per_table=on;
 set global innodb_file_format='Barracuda';

=== modified file 'sql/handler.cc'
--- a/sql/handler.cc	2011-11-16 14:12:49 +0000
+++ b/sql/handler.cc	2011-11-21 09:13:00 +0000
@@ -3799,7 +3799,6 @@ handler::check_if_supported_inplace_alte
     Alter_inplace_info::ADD_PK_INDEX |
     Alter_inplace_info::DROP_PK_INDEX;
   Alter_inplace_info::HA_ALTER_FLAGS inplace_offline_operations=
-    Alter_inplace_info::CHANGE_COLUMN |
     Alter_inplace_info::ALTER_COLUMN_NAME |
     Alter_inplace_info::COLUMN_DEFAULT_VALUE |
     Alter_inplace_info::CHANGE_CREATE_OPTION;

=== modified file 'sql/handler.h'
--- a/sql/handler.h	2011-11-16 14:12:49 +0000
+++ b/sql/handler.h	2011-11-21 09:13:00 +0000
@@ -927,6 +927,14 @@ public:
 };
 
 
+/**
+  Class describing changes to be done by ALTER TABLE.
+  Instance of this class is passed to storage engine in order
+  to determine if this ALTER TABLE can be done using in-place
+  algorithm. It is also used for executing the ALTER TABLE
+  using in-place algorithm.
+*/
+
 class Alter_inplace_info
 {
 public:
@@ -939,6 +947,11 @@ public:
      be performed by copying the table to a temporary table, will not
      have their own flags here (e.g. ALTER TABLE FORCE, ALTER TABLE
      ENGINE).
+
+     We generally try to specify handler flags only if there are real
+     changes. But in cases when it is cumbersome to determine if some
+     attribute has really changed we might choose to set flag
+     pessimistically, for example, relying on parser output only.
   */
   typedef ulong HA_ALTER_FLAGS;
 
@@ -966,36 +979,33 @@ public:
   // Drop column
   static const HA_ALTER_FLAGS DROP_COLUMN                = 1L << 7;
 
-  // General flag for CHANGE [COLUMN] | MODIFY [COLUMN], See ALTER_COLUMN_*
-  static const HA_ALTER_FLAGS CHANGE_COLUMN              = 1L << 8;
-
   // Rename column
-  static const HA_ALTER_FLAGS ALTER_COLUMN_NAME          = 1L << 9;
+  static const HA_ALTER_FLAGS ALTER_COLUMN_NAME          = 1L << 8;
 
   // Change column datatype
-  static const HA_ALTER_FLAGS ALTER_COLUMN_TYPE          = 1L << 10;
+  static const HA_ALTER_FLAGS ALTER_COLUMN_TYPE          = 1L << 9;
 
   // Reorder column
-  static const HA_ALTER_FLAGS ALTER_COLUMN_ORDER         = 1L << 11;
+  static const HA_ALTER_FLAGS ALTER_COLUMN_ORDER         = 1L << 10;
 
   // Change column from NULL to NOT NULL or vice versa
-  static const HA_ALTER_FLAGS ALTER_COLUMN_NULLABLE      = 1L << 12;
+  static const HA_ALTER_FLAGS ALTER_COLUMN_NULLABLE      = 1L << 11;
 
   // Set or remove default column value
-  static const HA_ALTER_FLAGS COLUMN_DEFAULT_VALUE       = 1L << 13;
+  static const HA_ALTER_FLAGS COLUMN_DEFAULT_VALUE       = 1L << 12;
 
   // Add foreign key
-  static const HA_ALTER_FLAGS ADD_FOREIGN_KEY            = 1L << 14;
+  static const HA_ALTER_FLAGS ADD_FOREIGN_KEY            = 1L << 13;
 
   // Drop foreign key
-  static const HA_ALTER_FLAGS DROP_FOREIGN_KEY           = 1L << 15;
+  static const HA_ALTER_FLAGS DROP_FOREIGN_KEY           = 1L << 14;
 
   // Change character set
-  static const HA_ALTER_FLAGS CHANGE_CHARACTER_SET       = 1L << 16;
-  static const HA_ALTER_FLAGS SET_DEFAULT_CHARACTER_SET  = 1L << 17;
+  static const HA_ALTER_FLAGS CHANGE_CHARACTER_SET       = 1L << 15;
+  static const HA_ALTER_FLAGS SET_DEFAULT_CHARACTER_SET  = 1L << 16;
 
   // table_options changed, see HA_CREATE_INFO::used_fields for details.
-  static const HA_ALTER_FLAGS CHANGE_CREATE_OPTION       = 1L << 18;
+  static const HA_ALTER_FLAGS CHANGE_CREATE_OPTION       = 1L << 17;
 
   KEY  *key_info_buffer;
   uint key_count;

=== modified file 'sql/sql_table.cc'
--- a/sql/sql_table.cc	2011-11-16 15:29:56 +0000
+++ b/sql/sql_table.cc	2011-11-21 09:13:00 +0000
@@ -5011,6 +5011,79 @@ err:
 
 
 /**
+  Check if key is a candidate key, i.e. a unique index with no index
+  fields partial or nullable.
+*/
+
+static bool is_candidate_key(KEY *key)
+{
+  KEY_PART_INFO *key_part;
+  KEY_PART_INFO *key_part_end= key->key_part + key->key_parts;
+
+  if (!(key->flags & HA_NOSAME))
+    return false;
+
+  for (key_part= key->key_part; key_part < key_part_end; key_part++)
+  {
+    if (key_part->field->maybe_null())
+      return false;
+    if (key_part->key_part_flag & HA_PART_KEY_SEG)
+      return false;
+  }
+
+  return true;
+}
+
+
+/**
+  Get Create_field object for newly created table by field index.
+
+  @param alter_info  Alter_info describing newly created table.
+  @param idx         Field index.
+*/
+
+static Create_field *get_field_by_index(Alter_info *alter_info, uint idx)
+{
+  List_iterator_fast<Create_field> field_it(alter_info->create_list);
+  uint field_idx= 0;
+  Create_field *field;
+
+  while ((field= field_it++) && field_idx < idx)
+  { field_idx++; }
+
+  return field;
+}
+
+
+/**
+  Auxiliary function that marks all existing fields in newly added key
+  with FIELD_IN_ADD_INDEX flag.
+*/
+
+static void mark_old_fields_in_added_key(Alter_info *alter_info, const KEY *key)
+{
+  const KEY_PART_INFO *key_part, *end;
+  Create_field *field;
+
+  key_part= key->key_part;
+  end= key_part + key->key_parts;
+  for(; key_part != end; key_part++)
+  {
+    field= get_field_by_index(alter_info, key_part->fieldnr);
+    /*
+      If the field has existed in old version of table mark it to
+      be part of new key.
+    */
+    if (field->field)
+      field->field->flags|= FIELD_IN_ADD_INDEX;
+  }
+}
+
+
+/**
+   Compare original and new versions of a table and fill Alter_inplace_info
+   describing differences between those versions.
+
    @param       thd                Thread
    @param       table              The original table.
    @param       alter_info         Alter options, fields and keys for the new
@@ -5025,10 +5098,13 @@ err:
 
    By comparing the changes between the original and new table
    we can determine how much it has changed after ALTER TABLE
-   and whether we need to make a copy of the table, or just change
-   the .frm file.
+   and whether storage engine can carry out this ALTER in-place.
 
    Mark any changes detected in the ha_alter_flags.
+   We generally try to specify handler flags only if there are real
+   changes. But in cases when it is cumbersome to determine if some
+   attribute has really changed we might choose to set flag
+   pessimistically, for example, relying on parser output only.
 
    If there are no data changes, but index changes, 'index_drop_buffer'
    and/or 'index_add_buffer' are populated with offsets into
@@ -5049,7 +5125,7 @@ static bool fill_alter_inplace_info(THD
   Field **f_ptr, *field;
   List_iterator_fast<Create_field> new_field_it, tmp_new_field_it;
   Create_field *new_field, *tmp_new_field;
-  KEY_PART_INFO *key_part;
+  KEY_PART_INFO *key_part, *new_part;
   KEY_PART_INFO *end;
   /*
     Remember if the new definition has new VARCHAR column;
@@ -5057,7 +5133,6 @@ static bool fill_alter_inplace_info(THD
   */
   bool varchar= create_info->varchar;
   uint candidate_key_count= 0;
-  bool not_nullable= true;
   DBUG_ENTER("fill_alter_inplace_info");
 
   /*
@@ -5096,20 +5171,14 @@ static bool fill_alter_inplace_info(THD
                             tmp_alter_info.key_list.elements)))
     DBUG_RETURN(true);
 
-  /*
-    First we setup ha_alter_flags based on what was detected
-    by parser
-  */
+  /* First we setup ha_alter_flags based on what was detected by parser. */
   if (alter_info->flags & Alter_info::ALTER_ADD_COLUMN)
     ha_alter_info->handler_flags|= Alter_inplace_info::ADD_COLUMN;
   if (alter_info->flags & Alter_info::ALTER_DROP_COLUMN)
     ha_alter_info->handler_flags|= Alter_inplace_info::DROP_COLUMN;
-  if (alter_info->flags & Alter_info::ALTER_CHANGE_COLUMN)
-    ha_alter_info->handler_flags|= Alter_inplace_info::CHANGE_COLUMN;
-  if (alter_info->flags & Alter_info::ALTER_CHANGE_COLUMN_DEFAULT)
+  if (alter_info->flags & (Alter_info::ALTER_CHANGE_COLUMN |
+                           Alter_info::ALTER_CHANGE_COLUMN_DEFAULT))
     ha_alter_info->handler_flags|= Alter_inplace_info::COLUMN_DEFAULT_VALUE;
-  if (alter_info->flags & Alter_info::ALTER_COLUMN_ORDER)
-    ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_ORDER;
   if (alter_info->flags & Alter_info::ADD_FOREIGN_KEY)
     ha_alter_info->handler_flags|= Alter_inplace_info::ADD_FOREIGN_KEY;
   if (alter_info->flags & Alter_info::DROP_FOREIGN_KEY)
@@ -5117,111 +5186,142 @@ static bool fill_alter_inplace_info(THD
   if (alter_info->flags & Alter_info::ALTER_OPTIONS)
     ha_alter_info->handler_flags|= Alter_inplace_info::CHANGE_CREATE_OPTION;
 
-  /*
-    Some very basic checks. If number of fields changes, or the
-    handler, we need to run full ALTER TABLE. In the future
-    new fields can be added and old dropped without copy, but
-    not yet.
-
-    Test also that engine was not given during ALTER TABLE, or
-    we are force to run regular alter table (copy).
-    E.g. ALTER TABLE tbl_name ENGINE=MyISAM.
+  /* TODO: do we want to have separate SE flags for the below cases? */
+  if (create_info->used_fields & HA_CREATE_USED_CHARSET)
+    ha_alter_info->handler_flags|= Alter_inplace_info::CHANGE_CHARACTER_SET;
+  if (create_info->used_fields & HA_CREATE_USED_DEFAULT_CHARSET)
+    ha_alter_info->handler_flags|= Alter_inplace_info::SET_DEFAULT_CHARACTER_SET;
 
-    For the following ones we also want to run regular alter table:
-    ALTER TABLE tbl_name ORDER BY ..
-    ALTER TABLE tbl_name CONVERT TO CHARACTER SET ..
-
-    At the moment we can't handle altering temporary tables without a copy.
-    We also test if OPTIMIZE TABLE was given and was mapped to alter table.
-    In that case we always do full copy.
-  */
-  if (table->s->fields != alter_info->create_list.elements ||
-      table->s->db_type() != create_info->db_type ||
-      table->s->tmp_table ||
-      create_info->used_fields & HA_CREATE_USED_ENGINE ||
-      create_info->used_fields & HA_CREATE_USED_CHARSET ||
-      create_info->used_fields & HA_CREATE_USED_DEFAULT_CHARSET ||
-      (table->s->row_type != create_info->row_type) ||
-      create_info->used_fields & HA_CREATE_USED_PACK_KEYS ||
-      create_info->used_fields & HA_CREATE_USED_MAX_ROWS ||
-      (alter_info->flags & (Alter_info::ALTER_RECREATE |
-                            Alter_info::ADD_FOREIGN_KEY |
-                            Alter_info::DROP_FOREIGN_KEY)) ||
-      order_num ||
-      (table->s->frm_version < FRM_VER_TRUE_VARCHAR && varchar))
-  {
-    /*
-      Check what has changed and set alter_flags
-    */
-    if (table->s->fields < alter_info->create_list.elements)
-      ha_alter_info->handler_flags|= Alter_inplace_info::ADD_COLUMN;
-    else if (table->s->fields > alter_info->create_list.elements)
-      ha_alter_info->handler_flags|= Alter_inplace_info::DROP_COLUMN;
-    if (create_info->used_fields & HA_CREATE_USED_CHARSET)
-      ha_alter_info->handler_flags|= Alter_inplace_info::CHANGE_CHARACTER_SET;
-    if (create_info->used_fields & HA_CREATE_USED_DEFAULT_CHARSET)
-      ha_alter_info->handler_flags|= Alter_inplace_info::SET_DEFAULT_CHARACTER_SET;
-    /* TODO check for ADD/DROP FOREIGN KEY */
-    if (table->s->frm_version < FRM_VER_TRUE_VARCHAR && varchar)
-      ha_alter_info->handler_flags|=  Alter_inplace_info::ALTER_COLUMN_TYPE;
-  }
   /*
-    Use transformed info to evaluate possibility of fast ALTER TABLE
-    but use the preserved field to persist modifications.
+    If we altering table with old VARCHAR fields we will be automatically
+    upgrading VARCHAR column types.
   */
-  new_field_it.init(alter_info->create_list);
-  tmp_new_field_it.init(tmp_alter_info.create_list);
+  if (table->s->frm_version < FRM_VER_TRUE_VARCHAR && varchar)
+    ha_alter_info->handler_flags|=  Alter_inplace_info::ALTER_COLUMN_TYPE;
 
   /*
-    Go through fields and check if the original ones are compatible
-    with new table.
+    Go through fields in old version of table and detect changes to them.
+    We don't want to rely solely on Alter_info flags for this since:
+    a) new definition of column can be fully identical to the old one
+       despite the fact that this column is mentioned in MODIFY clause.
+    b) even if new column type differs from its old column from metadata
+       point of view, it might be identical from storage engine point
+       of view (e.g. when ENUM('a','b') is changed to ENUM('a','b',c')).
+    c) flags passed to storage engine contain more detailed information
+       about nature of changes than those provided from parser.
   */
-  for (f_ptr= table->field, new_field= new_field_it++,
-       tmp_new_field= tmp_new_field_it++;
-       (new_field && (field= *f_ptr));
-       f_ptr++, new_field= new_field_it++,
-       tmp_new_field= tmp_new_field_it++)
+  for (f_ptr= table->field; (field= *f_ptr); f_ptr++)
   {
-    /* Make sure we have at least the default charset in use. */
-    if (!new_field->charset)
-      new_field->charset= create_info->default_table_charset;
+    /*
+      Clear markers for renamed and indexed fields which we are going to
+      set later.
+    */
+    field->flags&= ~(FIELD_IS_RENAMED|FIELD_IN_ADD_INDEX);
 
-    /* Don't pack rows in old tables if the user has requested this. */
-    if (create_info->row_type == ROW_TYPE_DYNAMIC ||
-        (tmp_new_field->flags & BLOB_FLAG) ||
-        (tmp_new_field->sql_type == MYSQL_TYPE_VARCHAR &&
-         create_info->row_type != ROW_TYPE_FIXED))
-      create_info->table_options|= HA_OPTION_PACK_RECORD;
+    /* Use transformed info to evaluate flags for storage engine. */
+    uint new_field_index= 0;
+    tmp_new_field_it.init(tmp_alter_info.create_list);
+    while ((tmp_new_field= tmp_new_field_it++))
+    {
+      if (tmp_new_field->field == field)
+        break;
+      new_field_index++;
+    }
 
-    /* Check how fields have been modified */
-    if (alter_info->flags & Alter_info::ALTER_CHANGE_COLUMN)
+    if (tmp_new_field)
     {
-      uint table_changes_local= 0;
-      /* Evaluate changes bitmap and send to check_if_incompatible_data() */
-      if (!(table_changes_local= field->is_equal(tmp_new_field)))
+      /* Field is not dropped. Evaluate changes bitmap for it. */
+
+      /*
+        Check if type of column has changed to some incompatible type.
+
+        We don't have separate flag for case when IS_EQUAL_PACK_LENGTH
+        is returned as this value is a part of legacy
+        handler::check_if_incompatible_data() API and almost no storage
+        engine uses it at the moment.
+        The only exception is CSV engine that treats all tables passing
+        SQL-layer checks as compatible. I.e. it is possible to change
+        length of VARCHAR() columns for CSV engine without rebuild.
+        TODO: Does it worth separate flag?
+      */
+      if ((field->is_equal(tmp_new_field) != IS_EQUAL_YES))
         ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_TYPE;
 
       /* Check if field was renamed */
-      field->flags&= ~FIELD_IS_RENAMED;
-      if (my_strcasecmp(system_charset_info,
-                        field->field_name,
+      if (my_strcasecmp(system_charset_info, field->field_name,
                         tmp_new_field->field_name))
       {
         field->flags|= FIELD_IS_RENAMED;
         ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_NAME;
       }
 
-      if (table_changes_local == IS_EQUAL_PACK_LENGTH)
-        ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_TYPE;
-
       /* Check that NULL behavior is same for old and new fields */
       if ((tmp_new_field->flags & NOT_NULL_FLAG) !=
           (uint) (field->flags & NOT_NULL_FLAG))
         ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_NULLABLE;
+
+      /*
+        Comparing new and old default values of column is cumbersome.
+        So instead of using such a comparison for detecting if default
+        has really changed we rely on flags set by parser to get an
+        approximate value for storage engine flag. This is done before
+        looping over individual columns.
+      */
+
+      /*
+        Detect changes in column order.
+      */
+      if (field->field_index != new_field_index)
+        ha_alter_info->handler_flags|= Alter_inplace_info::ALTER_COLUMN_ORDER;
+    }
+    else
+    {
+      /*
+        Field is not present in new version of table and therefore was dropped.
+        Corresponding storage engine flag should be already set.
+      */
+      DBUG_ASSERT(ha_alter_info->handler_flags & Alter_inplace_info::DROP_COLUMN);
     }
+  }
 
-    /* Clear indexed marker */
-    field->flags&= ~FIELD_IN_ADD_INDEX;
+#ifndef DBUG_OFF
+  tmp_new_field_it.init(tmp_alter_info.create_list);
+  while ((tmp_new_field= tmp_new_field_it++))
+  {
+    if (! tmp_new_field->field)
+    {
+      /*
+        Field is not present in old version of table and therefore was added.
+        Again corresponding storage engine flag should be already set.
+      */
+      DBUG_ASSERT(ha_alter_info->handler_flags & Alter_inplace_info::ADD_COLUMN);
+      break;
+    }
+  }
+#endif /* DBUG_OFF */
+
+  /*
+    Perform some modifications to original Alter_info and HA_CREATE_INFO.
+
+    TODO: Are these changes really necessary and belong here at all?
+  */
+  new_field_it.init(alter_info->create_list);
+  while ((new_field= new_field_it++))
+  {
+    /* Make sure we have at least the default charset in use. */
+    if (!new_field->charset)
+      new_field->charset= create_info->default_table_charset;
+  }
+
+  tmp_new_field_it.init(tmp_alter_info.create_list);
+  while ((tmp_new_field= tmp_new_field_it++))
+  {
+    /* Don't pack rows in old tables if the user has requested this. */
+    if (create_info->row_type == ROW_TYPE_DYNAMIC ||
+        (tmp_new_field->flags & BLOB_FLAG) ||
+        (tmp_new_field->sql_type == MYSQL_TYPE_VARCHAR &&
+         create_info->row_type != ROW_TYPE_FIXED))
+      create_info->table_options|= HA_OPTION_PACK_RECORD;
   }
 
   /*
@@ -5237,31 +5337,6 @@ static bool fill_alter_inplace_info(THD
   DBUG_PRINT("info", ("index count old: %d  new: %d",
                       table->s->keys, ha_alter_info->key_count));
 
-  /* Count all candidate keys. */
-
-  for (table_key= table->key_info; table_key < table_key_end; table_key++)
-  {
-    KEY_PART_INFO *table_part;
-    KEY_PART_INFO *table_part_end= table_key->key_part + table_key->key_parts;
-
-    /*
-      Check if key is a candidate key, i.e. a unique index with no index
-      fields nullable, then key is either already primary key or could
-      be promoted to primary key if the original primary key is dropped.
-    */
-    not_nullable= true;
-    bool prefix_key= false;
-    for (table_part= table_key->key_part;
-         table_part < table_part_end;
-         table_part++)
-    {
-      not_nullable= not_nullable && (! table_part->field->maybe_null());
-      prefix_key= prefix_key || (table_part->key_part_flag & HA_PART_KEY_SEG);
-    }
-    if ((table_key->flags & HA_NOSAME) && not_nullable && !prefix_key)
-      candidate_key_count++;
-  }
-
   /*
     Step through all keys of the old table and search matching new keys.
   */
@@ -5283,111 +5358,44 @@ static bool fill_alter_inplace_info(THD
       ha_alter_info->index_drop_buffer
         [ha_alter_info->index_drop_count++]=
         table_key;
-      if (table_key->flags & HA_NOSAME)
-      {
-        /* Unique key. Check for "PRIMARY". */
-        if ((uint) (table_key - table->key_info) == table->s->primary_key)
-        {
-          ha_alter_info->handler_flags|= Alter_inplace_info::DROP_PK_INDEX;
-          candidate_key_count--;
-        }
-        else
-        {
-          bool is_not_null= true;
-
-          ha_alter_info->handler_flags|= Alter_inplace_info::DROP_UNIQUE_INDEX;
-          key_part= table_key->key_part;
-          end= key_part + table_key->key_parts;
-
-         /*
-            Check if all fields in key are declared
-            NOT NULL and adjust candidate_key_count
-          */
-          for(; key_part != end; key_part++)
-          {
-            is_not_null=
-              (is_not_null &&
-               (!table->field[key_part->fieldnr-1]->maybe_null()));
-          }
-          if (is_not_null)
-            candidate_key_count--;
-        }
-      }
-      else
-        ha_alter_info->handler_flags|= Alter_inplace_info::DROP_INDEX;
       DBUG_PRINT("info", ("index dropped: '%s'", table_key->name));
       continue;
     }
 
-    bool is_not_null= true;
-    bool no_pk= ((table->s->primary_key == MAX_KEY) ||
-                 (ha_alter_info->handler_flags &
-                  Alter_inplace_info::DROP_PK_INDEX));
-    key_part= new_key->key_part;
-    end= key_part + new_key->key_parts;
-    for(; key_part != end; key_part++)
-    {
-      /*
-        Check if all fields in key are declared
-        NOT NULL
-      */
-      if (key_part->fieldnr < table->s->fields)
-      {
-        /* Mark field to be part of new key */
-        field= table->field[key_part->fieldnr];
-        field->flags|= FIELD_IN_ADD_INDEX;
-        is_not_null= (is_not_null && (!field->maybe_null()));
-      }
-      else
-      {
-        /* Index is defined over a newly added column */
-        List_iterator_fast<Create_field>
-          new_field_it(alter_info->create_list);
-        Create_field *new_field;
-        uint fieldnr;
-
-        for (fieldnr= 0, new_field= new_field_it++;
-             fieldnr != key_part->fieldnr;
-             fieldnr++, new_field= new_field_it++)
-          is_not_null=
-            (is_not_null && (new_field->flags & NOT_NULL_FLAG));
-      }
-    }
-
     /* Check that the key types are compatible between old and new tables. */
     if ((table_key->algorithm != new_key->algorithm) ||
         ((table_key->flags & HA_KEYFLAG_MASK) !=
          (new_key->flags & HA_KEYFLAG_MASK)) ||
         (table_key->key_parts != new_key->key_parts))
     {
-      if (table_key->flags & HA_NOSAME)
-      {
-        /* Unique key. Check for "PRIMARY". */
-        if ((uint) (table_key - table->key_info) == table->s->primary_key)
-        {
-          ha_alter_info->handler_flags|= Alter_inplace_info::DROP_PK_INDEX |
-                                         Alter_inplace_info::ADD_INDEX;
-        }
-        else
-        {
-          ha_alter_info->handler_flags|= Alter_inplace_info::DROP_UNIQUE_INDEX |
-                                         Alter_inplace_info::ADD_INDEX;
-        }
-      }
-      else
-      {
-        ha_alter_info->handler_flags|= Alter_inplace_info::DROP_INDEX;
-        if ((!my_strcasecmp(system_charset_info,
-                            new_key->name, primary_key_name)) ||
-            (no_pk && candidate_key_count == 0 && is_not_null &&
-             !(new_key->flags & HA_KEY_HAS_PART_KEY_SEG)))
-          ha_alter_info->handler_flags|= Alter_inplace_info::ADD_PK_INDEX;
-        else
-          ha_alter_info->handler_flags|= Alter_inplace_info::ADD_UNIQUE_INDEX;
-      }
       goto index_changed;
     }
 
+    /*
+      Check that the key parts remain compatible between the old and
+      new tables.
+    */
+    end= table_key->key_part + table_key->key_parts;
+    for (key_part= table_key->key_part, new_part= new_key->key_part;
+         key_part < end;
+         key_part++, new_part++)
+    {
+      /*
+        Key definition has changed if we are using a different field or
+        if the used key part length is different. It makes sense to
+        check lengths first as in case when fields differ it is likely
+        that lengths differ too and checking fields is more expensive
+        in general case.
+      */
+      if (key_part->length != new_part->length)
+        goto index_changed;
+
+      tmp_new_field= get_field_by_index(&tmp_alter_info, new_part->fieldnr);
+
+      if (! tmp_new_field->field || tmp_new_field->field != key_part->field)
+        goto index_changed;
+    }
+
     continue;
 
   index_changed:
@@ -5398,14 +5406,12 @@ static bool fill_alter_inplace_info(THD
     ha_alter_info->index_add_buffer
       [ha_alter_info->index_add_count++]=
       new_key - ha_alter_info->key_info_buffer;
-    key_part= new_key->key_part;
-    end= key_part + new_key->key_parts;
-    for(; key_part != end; key_part++)
-    {
-      /* Mark field to be part of new key */
-      if ((field= table->field[key_part->fieldnr]))
-        field->flags|= FIELD_IN_ADD_INDEX;
-    }
+    /* Mark all old fields which are used in newly created index. */
+    /*
+      TODO: Check with Cluster if this step is still necessary.
+      It doesn't make much sense to me...
+    */
+    mark_old_fields_in_added_key(&tmp_alter_info, new_key);
     DBUG_PRINT("info", ("index changed: '%s'", table_key->name));
   }
   /*end of for (; table_key < table_key_end;) */
@@ -5425,65 +5431,93 @@ static bool fill_alter_inplace_info(THD
     }
     if (table_key >= table_key_end)
     {
-      bool is_not_null= true;
-      bool no_pk= ((table->s->primary_key == MAX_KEY) ||
-                   (ha_alter_info->handler_flags &
-                    Alter_inplace_info::DROP_PK_INDEX));
-
       /* Key not found. Add the offset of the key to the add buffer. */
       ha_alter_info->index_add_buffer
         [ha_alter_info->index_add_count++]=
         new_key - ha_alter_info->key_info_buffer;
-      key_part= new_key->key_part;
-      end= key_part + new_key->key_parts;
-      for(; key_part != end; key_part++)
+      /* Mark all old fields which are used in newly created index. */
+      mark_old_fields_in_added_key(&tmp_alter_info, new_key);
+      DBUG_PRINT("info", ("index added: '%s'", new_key->name));
+    }
+  }
+
+  /* Now let us calculate flags for storage engine API. */
+
+  /* Count all existing candidate keys. */
+  for (table_key= table->key_info; table_key < table_key_end; table_key++)
+  {
+    /*
+      Check if key is a candidate key, This key is either already primary key
+      or could be promoted to primary key if the original primary key is
+      dropped.
+      In MySQL one is allowed to create primary key with partial fields (i.e.
+      primary key which is not considered candidate). For simplicity we count
+      such key as a candidate key here.
+    */
+    if (((uint) (table_key - table->key_info) == table->s->primary_key) ||
+        is_candidate_key(table_key))
+      candidate_key_count++;
+  }
+
+  /* Figure out what kind of indexes we are dropping. */
+  KEY **dropped_key;
+  KEY **dropped_key_end= ha_alter_info->index_drop_buffer +
+                         ha_alter_info->index_drop_count;
+
+  for (dropped_key= ha_alter_info->index_drop_buffer;
+       dropped_key < dropped_key_end; dropped_key++)
+  {
+    table_key= *dropped_key;
+
+    if (table_key->flags & HA_NOSAME)
+    {
+      /*
+        Unique key. Check for PRIMARY KEY. Also see comment about primary
+        and candidate keys above.
+      */
+      if ((uint) (table_key - table->key_info) == table->s->primary_key)
       {
-        /*
-          Check if all fields in key are declared
-          NOT NULL
-         */
-        if (key_part->fieldnr < table->s->fields)
-        {
-          /* Mark field to be part of new key */
-          field= table->field[key_part->fieldnr];
-          field->flags|= FIELD_IN_ADD_INDEX;
-          is_not_null= (is_not_null && (!field->maybe_null()));
-        }
-        else
-        {
-          /* Index is defined over a newly added column */
-          List_iterator_fast<Create_field>
-            new_field_it(alter_info->create_list);
-          Create_field *new_field;
-          uint fieldnr;
-
-          for (fieldnr= 0, new_field= new_field_it++;
-               fieldnr != key_part->fieldnr;
-               fieldnr++, new_field= new_field_it++)
-            is_not_null=
-              (is_not_null && (new_field->flags & NOT_NULL_FLAG));
-        }
+        ha_alter_info->handler_flags|= Alter_inplace_info::DROP_PK_INDEX;
+        candidate_key_count--;
+      }
+      else
+      {
+        ha_alter_info->handler_flags|= Alter_inplace_info::DROP_UNIQUE_INDEX;
+        if (is_candidate_key(table_key))
+          candidate_key_count--;
       }
-      if (new_key->flags & HA_NOSAME)
+    }
+    else
+      ha_alter_info->handler_flags|= Alter_inplace_info::DROP_INDEX;
+  }
+
+  /* Now figure out what kind of indexes we are adding. */
+  for (uint add_key_idx= 0; add_key_idx < ha_alter_info->index_add_count; add_key_idx++)
+  {
+    new_key= ha_alter_info->key_info_buffer + ha_alter_info->index_add_buffer[add_key_idx];
+
+    if (new_key->flags & HA_NOSAME)
+    {
+      bool is_pk= !my_strcasecmp(system_charset_info, new_key->name, primary_key_name);
+
+      if ((!(new_key->flags & HA_KEY_HAS_PART_KEY_SEG) &&
+           !(new_key->flags & HA_NULL_PART_KEY)) ||
+          is_pk)
       {
-        /* Unique key. Check for "PRIMARY"
-           or if adding first unique key
-           defined on non-nullable
-        */
-        DBUG_PRINT("info",("no_pk %s, candidate_key_count %u, is_not_null %s",
-                           (no_pk)?"yes":"no", candidate_key_count, (is_not_null)?"yes":"no"));
-        if ((!my_strcasecmp(system_charset_info,
-                            new_key->name, primary_key_name)) ||
-            (no_pk && candidate_key_count == 0 && is_not_null &&
-             !(new_key->flags & HA_KEY_HAS_PART_KEY_SEG)))
+        /* Candidate key or primary key! */
+        if (candidate_key_count == 0 || is_pk)
           ha_alter_info->handler_flags|= Alter_inplace_info::ADD_PK_INDEX;
         else
           ha_alter_info->handler_flags|= Alter_inplace_info::ADD_UNIQUE_INDEX;
+        candidate_key_count++;
       }
       else
-        ha_alter_info->handler_flags|= Alter_inplace_info::ADD_INDEX;
-      DBUG_PRINT("info", ("index added: '%s'", new_key->name));
+      {
+        ha_alter_info->handler_flags|= Alter_inplace_info::ADD_UNIQUE_INDEX;
+      }
     }
+    else
+      ha_alter_info->handler_flags|= Alter_inplace_info::ADD_INDEX;
   }
 
   DBUG_RETURN(false);
@@ -6013,15 +6047,31 @@ static bool is_inplace_alter_impossible(
 {
   DBUG_ENTER("is_inplace_alter_impossible");
 
-  if (table->s->tmp_table) // In-place not supported for tmp tables
+  /* At the moment we can't handle altering temporary tables without a copy. */
+  if (table->s->tmp_table)
     DBUG_RETURN(true);
 
+
+  /*
+    We also test if OPTIMIZE TABLE was given and was mapped to alter table.
+    In that case we always do full copy (ALTER_RECREATE is set in this case).
+
+    For the ALTER TABLE tbl_name ORDER BY ... we also want to run regular
+    alter table. TODO: Can be done in-place in theory?
+  */
   if (alter_info->flags & (Alter_info::ALTER_RECREATE |
                            Alter_info::ALTER_TABLE_REORG |
                            Alter_info::ALTER_RENAME |
                            Alter_info::ALTER_ORDER))
     DBUG_RETURN(true);
 
+  /*
+    Test also that engine was not given during ALTER TABLE, or
+    we are force to run regular alter table (copy).
+    E.g. ALTER TABLE tbl_name ENGINE=MyISAM.
+
+    TODO: Do non-engine flags can be done in-place?
+  */
   if (alter_info->flags & Alter_info::ALTER_OPTIONS)
   {
     if (create_info->db_type != table->s->db_type() ||

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-trunk-wl5534 branch (Dmitry.Lenev:3427 to 3428) WL#5534Dmitry Lenev21 Nov