List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:November 21 2011 3:24pm
Subject:bzr push into mysql-trunk-wl5534 branch (Dmitry.Lenev:3428 to 3429) WL#5534
View as plain text  
 3429 Dmitry Lenev	2011-11-21
      WL#5534 Online ALTER, Phase 1.
      
      Patch #52: Review changes.
      
      Move code adjusting HA_CREATE_INFO and Alter_info
      for in-place ALTER out of fill_alter_inplace_info().
      So the latter doesn't have unobvious side-effect.

    modified:
      sql/sql_table.cc
 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
=== modified file 'sql/sql_table.cc'
--- a/sql/sql_table.cc	2011-11-21 09:13:00 +0000
+++ b/sql/sql_table.cc	2011-11-21 15:23:55 +0000
@@ -3011,6 +3011,10 @@ mysql_prepare_create_table(THD *thd, HA_
       executing a prepared statement for the second time.
     */
     sql_field->length= sql_field->char_length;
+    /*
+      Set field charset. See also set_default_charset_for_fields()
+      which mimics behavior of this code for case of in-place ALTER.
+    */
     save_cs= sql_field->charset= get_sql_field_charset(sql_field,
                                                        create_info);
     if ((sql_field->flags & BINCMP_FLAG) &&
@@ -5091,6 +5095,8 @@ static void mark_old_fields_in_added_key
    @param       create_info        Create options for the new table.
    @param       order_num          Number of order list elements.
    @param[out]  ha_alter_info      Data structures needed for in-place alter
+   @param[out]  pack_record        Indicates if new version of table will use
+                                   packed records.
 
    First argument 'table' contains information of the original
    table, which includes all corresponding parts that the new
@@ -5120,11 +5126,12 @@ static bool fill_alter_inplace_info(THD
                                     Alter_info *alter_info,
                                     HA_CREATE_INFO *create_info,
                                     uint order_num,
-                                    Alter_inplace_info *ha_alter_info)
+                                    Alter_inplace_info *ha_alter_info,
+                                    bool *pack_record)
 {
   Field **f_ptr, *field;
   List_iterator_fast<Create_field> new_field_it, tmp_new_field_it;
-  Create_field *new_field, *tmp_new_field;
+  Create_field *tmp_new_field;
   KEY_PART_INFO *key_part, *new_part;
   KEY_PART_INFO *end;
   /*
@@ -5151,7 +5158,7 @@ static bool fill_alter_inplace_info(THD
     destroy the copy.
   */
   Alter_info tmp_alter_info(*alter_info, thd->mem_root);
-  uint db_options= 0; /* not used */
+  uint db_options= 0;
 
   /* Create the prepared information. */
   if (mysql_prepare_create_table(thd, create_info,
@@ -5171,6 +5178,18 @@ static bool fill_alter_inplace_info(THD
                             tmp_alter_info.key_list.elements)))
     DBUG_RETURN(true);
 
+  /*
+    When storage engine evaluates if in-place alter is possible it needs
+    HA_OPTION_PACK_RECORD flag correctly set for old and new versions of
+    table. Unfortunately, with current code it is impossible to evaluate
+    proper value for this flag for new version of the table without
+    calling mysql_prepare_create_table() or simulating its behavior.
+    To avoid doing this we piggyback on the above call information needed
+    to calculate correct value of this flag and return it as an out
+    parameter.
+  */
+  *pack_record= (db_options & HA_OPTION_PACK_RECORD);
+
   /* 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;
@@ -5301,30 +5320,6 @@ static bool fill_alter_inplace_info(THD
 #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;
-  }
-
-  /*
     Go through keys and check if the original ones are compatible
     with new table.
   */
@@ -6919,6 +6914,28 @@ static int simple_rename_or_index_change
 
 
 /**
+  Auxiliary function which sets default charset for all new fields
+  if no explicit charset is provided.
+
+  @note This function mimics behavior of charset handling code from
+        mysql_prepare_create_table() and has to be changed if this
+        code changes.
+*/
+
+static void set_default_charset_for_fields(HA_CREATE_INFO *create_info,
+                                           Alter_info *alter_info)
+{
+  List_iterator_fast<Create_field> field_it(alter_info->create_list);
+  Create_field *field;
+  while ((field= field_it++))
+  {
+    if (!field->charset)
+      field->charset= create_info->default_table_charset;
+  }
+}
+
+
+/**
   Alter table
 
   @param thd              Thread handle
@@ -7285,15 +7302,28 @@ bool mysql_alter_table(THD *thd,char *ne
   {
     Alter_inplace_info ha_alter_info(ignore);
     bool use_inplace= true;
+    bool pack_record;
 
     /* Fill the Alter_inplace_info structure. */
     if (fill_alter_inplace_info(thd, table, alter_info,
                                 create_info, order_num,
-                                &ha_alter_info))
+                                &ha_alter_info,
+                                &pack_record))
     {
       DBUG_RETURN(true);
     }
 
+    /*
+      Perform some modifications to original Alter_info and HA_CREATE_INFO
+      to allow check_if_supported_inplace_alter() work correctly.
+
+      First, make sure that all fields have at least default charset set.
+    */
+    set_default_charset_for_fields(create_info, alter_info);
+    /* Second, calculate correct value of HA_OPTION_PACK_RECORD flag. */
+    if (create_info->row_type == ROW_TYPE_DYNAMIC || pack_record)
+      create_info->table_options|= HA_OPTION_PACK_RECORD;
+
     enum_alter_inplace_result inplace_supported;
     if (ha_alter_info.handler_flags == 0) // This shouldn't really happen...
       inplace_supported= HA_ALTER_INPLACE_NOT_SUPPORTED;

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