From: Dmitry Lenev Date: November 21 2011 3:24pm Subject: bzr push into mysql-trunk-wl5534 branch (Dmitry.Lenev:3428 to 3429) WL#5534 List-Archive: http://lists.mysql.com/commits/142101 Message-Id: <20111121152423.1D42D421FF4@jubjub> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 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 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).