MySQL Bugs: #33650: mysql_alter_table() unnecessarily does full table copy

Bug #33650	mysql_alter_table() unnecessarily does full table copy
Submitted:	3 Jan 2008 12:38	Modified:	10 May 2011 13:53
Reporter:	Marko Mäkelä	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: DDL	Severity:	S5 (Performance)
Version:	5.1.23-bk, 5.5.7	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
MySQL does a full table copy when creating an index on a UTF-8 VARCHAR column.

How to repeat:
create table t1(a int, b varchar(255), primary key(a,b))
engine=myisam default charset=utf8;
create index t1ba on t1(b,a);

Suggested fix:
Investigate why this is_equal test in compare_tables() fails on column b:

    /* Evaluate changes bitmap and send to check_if_incompatible_data()
    */
    if (!(tmp= field->is_equal(new_field)))
    {
       *need_copy_table= ALTER_TABLE_DATA_CHANGED;
       DBUG_RETURN(0);
    }

Many thanks for writing a bug report.
I'll set these to feature request, because I can't see a behaviour failure.

Vasil,

please look at MySQL code and suggest a patch to MySQL AB. This is an important bug.

Regards,

Heikki

Here is a simpler test case:

create table t1(b varchar(10)) engine=myisam default charset=utf8;
create index t1b on t1(b);

in compare_tables():

Breakpoint 3, compare_tables (table=0x803f53000, alter_info=0x7ffffebb5040, 
    create_info=0x7ffffebb4750, order_num=0, need_copy_table=0x7ffffebb43ac, 
    key_info_buffer=0x7ffffebb43d0, index_drop_buffer=0x7ffffebb43c0, 
    index_drop_count=0x7ffffebb43cc, index_add_buffer=0x7ffffebb43b0, 
    index_add_count=0x7ffffebb43bc) at sql_table.cc:5075
5075	  uint changes= 0, tmp;
(gdb) ins ((Create_field*)(alter_info->create_list->first->info))->length
$37 = 10
(gdb) ins table->field->field_length 
$38 = 30

subsequently, this check fails because the above are not equal:

sql/sql_table.cc:5197     /* Evaluate changes bitmap and send to check_if_incompatible_data() */
sql/sql_table.cc:5198     if (!(tmp= field->is_equal(new_field)))
sql/sql_table.cc:5199     {
sql/sql_table.cc:5200       *need_copy_table= ALTER_TABLE_DATA_CHANGED;
sql/sql_table.cc:5201       DBUG_RETURN(0);

this is the body of the is_equal() method where 10 is compared with 30 (line 7467):

sql/field.cc:7462 uint Field_varstring::is_equal(Create_field *new_field)
sql/field.cc:7463 {
sql/field.cc:7464   if (new_field->sql_type == real_type() &&
sql/field.cc:7465       new_field->charset == field_charset)
sql/field.cc:7466   {
sql/field.cc:7467     if (new_field->length == max_display_length())
sql/field.cc:7468       return IS_EQUAL_YES;
sql/field.cc:7469     if (new_field->length > max_display_length() &&
sql/field.cc:7470         ((new_field->length <= 255 && max_display_length() <= 255) ||
sql/field.cc:7471          (new_field->length > 255 && max_display_length() > 255)))
sql/field.cc:7472       return IS_EQUAL_PACK_LENGTH; // VARCHAR, longer variable length
sql/field.cc:7473   }
sql/field.cc:7474   return IS_EQUAL_NO;
sql/field.cc:7475 }

compare_tables() is called from:

(gdb) bt
#0  compare_tables (table=0x803f53000, alter_info=0x7ffffebb5040, 
    create_info=0x7ffffebb4750, order_num=0, need_copy_table=0x7ffffebb43ac, 
    key_info_buffer=0x7ffffebb43d0, index_drop_buffer=0x7ffffebb43c0, 
    index_drop_count=0x7ffffebb43cc, index_add_buffer=0x7ffffebb43b0, 
    index_add_count=0x7ffffebb43bc) at sql_table.cc:5075
#1  0x00000000006dd92f in mysql_alter_table (thd=0x803063000, new_db=0x803f14420 "test", 
    new_name=0x803f140c0 "t1", create_info=0x7ffffebb4750, table_list=0x803f140f8, 
    alter_info=0x7ffffebb5040, order_num=0, order=0x0, ignore=false) at sql_table.cc:6267
#2  0x00000000005cbec9 in mysql_execute_command (thd=0x803063000) at sql_parse.cc:2564
#3  0x00000000005d2612 in mysql_parse (thd=0x803063000, 
    inBuf=0x803f14010 "create index t1b on t1(b)", length=25, 
    found_semicolon=0x7ffffebb5de0) at sql_parse.cc:5629
#4  0x00000000005d3224 in dispatch_command (command=COM_QUERY, thd=0x803063000, 
    packet=0x803079001 "create index t1b on t1(b)", packet_length=25) at sql_parse.cc:1121
#5  0x00000000005d435f in do_command (thd=0x803063000) at sql_parse.cc:781
#6  0x00000000005c43cc in handle_one_connection (arg=0x803063000) at sql_connect.cc:1120
#7  0x0000000800e639a8 in pthread_getprio () from /lib/libthr.so.3

In mysql_alter_table():

sql/sql_table.cc:6267     if (compare_tables(table, alter_info,
sql/sql_table.cc:6268                        create_info, order_num,
sql/sql_table.cc:6269                        &need_copy_table_res,
sql/sql_table.cc:6270                        &key_info_buffer,
sql/sql_table.cc:6271                        &index_drop_buffer, &index_drop_count,
sql/sql_table.cc:6272                        &index_add_buffer, &index_add_count))

"table" comes from

sql/sql_table.cc:5987   if (!(table= open_n_lock_single_table(thd, table_list, TL_WRITE_ALLOW_READ)))

and has table->field->field_length == 30

"alter_info: comes from

sql/sql_table.cc:6249   if (mysql_prepare_alter_table(thd, table, create_info, alter_info))

and has ((Create_field*)(alter_info->create_list->first->info))->length == 10

inside mysql_prepare_alter_table():

sql/sql_table.cc:5534       def= new Create_field(field, field);

here field->field_length == 30, but the created object has length == 10, e.g. def->length == 10 after this call. This is because of this code in the Create_field() constructor:

sql/field.cc:10019   case MYSQL_TYPE_VAR_STRING:
sql/field.cc:10020     /* This is corrected in create_length_to_internal_length */
sql/field.cc:10021     length= (length+charset->mbmaxlen-1) / charset->mbmaxlen;

About the fix:

I think that the create_length_to_internal_length() method needs to be called but I am not sure where. In order to minimize the effect of the invocation it can be called from just before the is_equal() check. Or it can be called after the Create_field() constructor or at the end of it.

This is the execution path with the attached patch applied:

Breakpoint 1, compare_tables (table=0x803f53000, alter_info=0x7ffffebb5040, 
    create_info=0x7ffffebb4750, order_num=0, need_copy_table=0x7ffffebb43ac, 
    key_info_buffer=0x7ffffebb43d0, index_drop_buffer=0x7ffffebb43c0, 
    index_drop_count=0x7ffffebb43cc, index_add_buffer=0x7ffffebb43b0, 
    index_add_count=0x7ffffebb43bc) at sql_table.cc:5198
5198	    new_field->create_length_to_internal_length();
(gdb) ins new_field->length
$1 = 10
(gdb) n
5199	    if (!(tmp= field->is_equal(new_field)))
(gdb) ins new_field->length
$2 = 30
(gdb) n
5205	    field->flags&= ~FIELD_IN_ADD_INDEX;
(gdb) l 5201
5196	
5197	    /* Evaluate changes bitmap and send to check_if_incompatible_data() */
5198	    new_field->create_length_to_internal_length();
5199	    if (!(tmp= field->is_equal(new_field)))
5200	    {
5201	      *need_copy_table= ALTER_TABLE_DATA_CHANGED;
5202	      DBUG_RETURN(0);
5203	    }
5204	    // Clear indexed marker
5205	    field->flags&= ~FIELD_IN_ADD_INDEX;

I.e. it fixes this bug, but I am not sure if it does not introduce any other bugs. To be sure it would be best to convert back that length to 10 after the is_equal() check, after line 5203 above. Or maybe this is not necessary.

Someone more familiar with the code should take a look and either confirm that this patch does not introduce new bugs and commit it or develop a new one. De-assigning this from myself.

Patch that fixes this bug by calling create_length_to_internal_length() just before the is_equal() check

Attachment: bug33650.diff (application/octet-stream, text), 420 bytes.

See also Bug #39833.

5.1 and 6.0 will need different fixes, since the online alter table algorithm was changed substantially in 6.0.
The effort is medium, initially.
Davi, here's how I think this could be fixed:
- we need to separate in Create_field the length in characters that we get from parser from octet length that we pass to the storage engine. I.e. we need to introduce an octet length member and maintain it throughout Create_field life cycle and in other places. This can be an arduous job, given our current mysql_alter_table() mess.

Sveta, could you please verify it against 5.5.7?
Thanks,
-- 
kostja

Bug is still repeatable with mysql-trunk

Changes to test case. No changelog entry needed.

CHANGESET - http://lists.mysql.com/commits/136982

This ticket is "Closed", but the documentation for mysql 5.5 still links to it: http://dev.mysql.com/doc/innodb/1.1/en/innodb-create-index-limitations.html

Is this a bug in 5.5 or not, and if so shouldn't this ticket be open?

Greg, looks like a misunderstanding since only MyISAM wasn't using fast alter table and MyISAM didn't support fast alter table at all. No effect on InnoDB which worked and continues to work. The documentation team should update the documentation in a little while. Thanks for letting us know!