Hello Tatiana,
I've discussed this fix with Bar.
The main conclusion is: probably the addition in the Create_feld::init() is enough,
but code removal in the make_field() is not needed.
Also please see my commentaries below.
Thank you,
Gleb.
Tatiana A. Nurnberg wrote:
> #At file:///misc/mysql/forest/43508/51-43508/ based on
> revid:aelkin@stripped
>
> 2973 Tatiana A. Nurnberg 2009-06-24
> Bug#43508: Renaming timestamp or date column triggers table copy
>
> Altering a table to update a column with types DATE or TIMESTAMP
> would incorrectly be seen as a significant change that necessitates
> a slow copy+rename operation instead of a fast update.
>
> There were two problems:
>
> The character set is magically set for TIMESTAMP to be "binary",
> but that was done too deep in field use code for ALTER TABLE to
> know of it. Now, put that in the constructor for Field_timestamp.
>
> Also, when we set the character set for the new replacement/
> comparison field, also raise the "binary" field flag that tells us
> we should compare it exactly. That is necessary to match the old
> stored definition.
>
> Next is the problem that the default length for TIMESTAMP and DATE
> fields is different than the length read from the .frm . The
> compressed size is written to the file, but the human-readable,
> part-delimited length is used as default length. IIRC, for
> timestamp it was 19!=14, and for date it was 8!=10. Length
> mismatch causes a table copy.
>
> Also, clean up a place where a comparison function alters one of its
> parameters and replace it with an assertion of the condition it
> mutates.
>
> modified:
> sql/field.cc
> sql/field.h
> sql/sql_table.cc
Can you please add per-file commentaries?
IMO changes in sql_table.cc are not necessary at all -- AFAIU it is just
a sort of refactoring, but without the per-file commentary it is not obvious.
> === modified file 'sql/field.cc'
> --- a/sql/field.cc 2009-06-09 16:44:26 +0000
> +++ b/sql/field.cc 2009-06-24 11:02:20 +0000
> @@ -1,4 +1,4 @@
> -/* Copyright 2000-2008 MySQL AB, 2008 Sun Microsystems, Inc.
> +/* Copyright 2000-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
>
> 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
> @@ -4723,6 +4723,7 @@ Field_timestamp::Field_timestamp(uchar *
> {
> /* For 4.0 MYD and 4.0 InnoDB compatibility */
> flags|= ZEROFILL_FLAG | UNSIGNED_FLAG;
> + field_charset= &my_charset_bin;
> if (!share->timestamp_field && unireg_check != NONE)
> {
> /* This timestamp has auto-update */
> @@ -4743,6 +4744,7 @@ Field_timestamp::Field_timestamp(bool ma
> {
> /* For 4.0 MYD and 4.0 InnoDB compatibility */
> flags|= ZEROFILL_FLAG | UNSIGNED_FLAG;
> + field_charset= &my_charset_bin;
> if (unireg_check != TIMESTAMP_DN_FIELD)
> flags|= ON_UPDATE_NOW_FLAG;
> }
It seems like these two additional initializations are not necessary.
OTOH if you change Field_timestamp constructors, why not to change
Field_date, Field_newdate, Field_time and Field_datetime?
> @@ -6504,20 +6506,9 @@ uint Field::is_equal(Create_field *new_f
> }
>
>
> -/* If one of the fields is binary and the other one isn't return 1 else 0 */
> -
> -bool Field_str::compare_str_field_flags(Create_field *new_field, uint32 flag_arg)
> -{
> - return (((new_field->flags & (BINCMP_FLAG | BINARY_FLAG)) &&
> - !(flag_arg & (BINCMP_FLAG | BINARY_FLAG))) ||
> - (!(new_field->flags & (BINCMP_FLAG | BINARY_FLAG)) &&
> - (flag_arg & (BINCMP_FLAG | BINARY_FLAG))));
> -}
> -
> -
> uint Field_str::is_equal(Create_field *new_field)
> {
> - if (compare_str_field_flags(new_field, flags))
> + if (field_flags_are_binary() != new_field->field_flags_are_binary())
> return 0;
>
> return ((new_field->sql_type == real_type()) &&
> @@ -8283,7 +8274,7 @@ uint Field_blob::max_packed_col_length(u
>
> uint Field_blob::is_equal(Create_field *new_field)
> {
> - if (compare_str_field_flags(new_field, flags))
> + if (field_flags_are_binary() != new_field->field_flags_are_binary())
> return 0;
>
> return ((new_field->sql_type == get_blob_type_from_length(max_data_length()))
> @@ -9569,7 +9560,7 @@ bool Create_field::init(THD *thd, char *
> }
>
> if (length == 0)
> - fld_length= 0; /* purecov: inspected */
> + fld_length= NULL; /* purecov: inspected */
> }
>
> sign_len= fld_type_modifier & UNSIGNED_FLAG ? 0 : 1;
> @@ -9721,8 +9712,7 @@ bool Create_field::init(THD *thd, char *
> case MYSQL_TYPE_TIMESTAMP:
> if (fld_length == NULL)
> {
> - /* Compressed date YYYYMMDDHHMMSS */
> - length= MAX_DATETIME_COMPRESSED_WIDTH;
> + length= MAX_DATETIME_WIDTH;
> }
> else if (length != MAX_DATETIME_WIDTH)
> {
> @@ -9787,7 +9777,7 @@ bool Create_field::init(THD *thd, char *
> sql_type= MYSQL_TYPE_NEWDATE;
> /* fall trough */
> case MYSQL_TYPE_NEWDATE:
> - length= 10;
> + length= MAX_DATE_WIDTH;
> break;
> case MYSQL_TYPE_TIME:
> length= 10;
> @@ -9868,6 +9858,17 @@ bool Create_field::init(THD *thd, char *
> DBUG_RETURN(TRUE);
> }
>
> + switch (fld_type) {
> + case MYSQL_TYPE_DATE:
> + case MYSQL_TYPE_NEWDATE:
> + case MYSQL_TYPE_TIME:
> + case MYSQL_TYPE_DATETIME:
> + case MYSQL_TYPE_TIMESTAMP:
> + charset= &my_charset_bin;
> + flags|= BINCMP_FLAG;
> + default: break;
> + }
> +
> DBUG_RETURN(FALSE); /* success */
> }
>
> @@ -9976,16 +9977,6 @@ Field *make_field(TABLE_SHARE *share, uc
> null_bit= ((uchar) 1) << null_bit;
> }
>
> - switch (field_type) {
> - case MYSQL_TYPE_DATE:
> - case MYSQL_TYPE_NEWDATE:
> - case MYSQL_TYPE_TIME:
> - case MYSQL_TYPE_DATETIME:
> - case MYSQL_TYPE_TIMESTAMP:
> - field_charset= &my_charset_bin;
> - default: break;
> - }
> -
I think it is better to keep these lines above for the
compatibility with old .frm (before this fix) that still
contain utf8_general_ci instead of binary charset (and to
save old behavior).
> if (f_is_alpha(pack_flag))
> {
> if (!f_is_packed(pack_flag))
>
> === modified file 'sql/field.h'
> --- a/sql/field.h 2009-06-09 16:44:26 +0000
> +++ b/sql/field.h 2009-06-24 11:02:20 +0000
> @@ -1,4 +1,4 @@
> -/* Copyright 2000-2008 MySQL AB, 2008 Sun Microsystems, Inc.
> +/* Copyright 2000-2008 MySQL AB, 2008, 2009 Sun Microsystems, Inc.
>
> 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
> @@ -603,6 +603,12 @@ protected:
> handle_int64(to, from, low_byte_first_from, table->s->db_low_byte_first);
> return from + sizeof(int64);
> }
> +
> + bool field_flags_are_binary()
> + {
> + return (flags & (BINCMP_FLAG | BINARY_FLAG)) != 0;
> + }
> +
> };
>
>
> @@ -658,7 +664,6 @@ public:
> friend class Create_field;
> my_decimal *val_decimal(my_decimal *);
> virtual bool str_needs_quotes() { return TRUE; }
> - bool compare_str_field_flags(Create_field *new_field, uint32 flags);
> uint is_equal(Create_field *new_field);
> };
>
> @@ -1268,12 +1273,12 @@ public:
> Field_date(uchar *ptr_arg, uchar *null_ptr_arg, uchar null_bit_arg,
> enum utype unireg_check_arg, const char *field_name_arg,
> CHARSET_INFO *cs)
> - :Field_str(ptr_arg, 10, null_ptr_arg, null_bit_arg,
> + :Field_str(ptr_arg, MAX_DATE_WIDTH, null_ptr_arg, null_bit_arg,
> unireg_check_arg, field_name_arg, cs)
> {}
> Field_date(bool maybe_null_arg, const char *field_name_arg,
> CHARSET_INFO *cs)
> - :Field_str((uchar*) 0,10, maybe_null_arg ? (uchar*) "": 0,0,
> + :Field_str((uchar*) 0, MAX_DATE_WIDTH, maybe_null_arg ? (uchar*) "": 0,0,
> NONE, field_name_arg, cs) {}
> enum_field_types type() const { return MYSQL_TYPE_DATE;}
> enum ha_base_keytype key_type() const { return HA_KEYTYPE_ULONG_INT; }
> @@ -1383,12 +1388,12 @@ public:
> Field_datetime(uchar *ptr_arg, uchar *null_ptr_arg, uchar null_bit_arg,
> enum utype unireg_check_arg, const char *field_name_arg,
> CHARSET_INFO *cs)
> - :Field_str(ptr_arg, 19, null_ptr_arg, null_bit_arg,
> + :Field_str(ptr_arg, MAX_DATETIME_WIDTH, null_ptr_arg, null_bit_arg,
> unireg_check_arg, field_name_arg, cs)
> {}
> Field_datetime(bool maybe_null_arg, const char *field_name_arg,
> CHARSET_INFO *cs)
> - :Field_str((uchar*) 0,19, maybe_null_arg ? (uchar*) "": 0,0,
> + :Field_str((uchar*) 0, MAX_DATETIME_WIDTH, maybe_null_arg ? (uchar*) "": 0,0,
> NONE, field_name_arg, cs) {}
> enum_field_types type() const { return MYSQL_TYPE_DATETIME;}
> #ifdef HAVE_LONG_LONG
> @@ -2061,6 +2066,11 @@ public:
> Item *on_update_value, LEX_STRING *comment, char *change,
> List<String> *interval_list, CHARSET_INFO *cs,
> uint uint_geom_type);
> +
> + bool field_flags_are_binary()
> + {
> + return (flags & (BINCMP_FLAG | BINARY_FLAG)) != 0;
> + }
> };
>
>
>
> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc 2009-06-19 08:24:43 +0000
> +++ b/sql/sql_table.cc 2009-06-24 11:02:20 +0000
> @@ -2468,8 +2468,10 @@ mysql_prepare_create_table(THD *thd, HA_
> executing a prepared statement for the second time.
> */
> sql_field->length= sql_field->char_length;
> - if (!sql_field->charset)
> +
> + if (sql_field->charset == NULL)
> sql_field->charset= create_info->default_table_charset;
> +
> /*
> table_charset is set in ALTER TABLE if we want change character set
> for all varchar/char columns.
> @@ -5470,8 +5472,8 @@ compare_tables(TABLE *table,
> Field **f_ptr, *field;
> uint changes= 0, tmp;
> uint key_count;
> - List_iterator_fast<Create_field> new_field_it, tmp_new_field_it;
> - Create_field *new_field, *tmp_new_field;
> + List_iterator_fast<Create_field> tmp_new_field_it;
> + Create_field *tmp_new_field;
> KEY_PART_INFO *key_part;
> KEY_PART_INFO *end;
> THD *thd= table->in_use;
> @@ -5497,6 +5499,9 @@ compare_tables(TABLE *table,
> pass it to mysql_prepare_create_table, then use the result
> to evaluate possibility of fast ALTER TABLE, and then
> destroy the copy.
> +
> + We shouldn't need to refer later in the function to alter_info
> + after this step.
> */
> Alter_info tmp_alter_info(*alter_info, thd->mem_root);
> uint db_options= 0; /* not used */
> @@ -5508,6 +5513,7 @@ compare_tables(TABLE *table,
> table->file, key_info_buffer,
> &key_count, 0))
> DBUG_RETURN(1);
> +
> /* Allocate result buffers. */
> if (! (*index_drop_buffer=
> (uint*) thd->alloc(sizeof(uint) * table->s->keys)) ||
> @@ -5541,7 +5547,7 @@ compare_tables(TABLE *table,
> prior to 5.0 branch.
> See BUG#6236.
> */
> - if (table->s->fields != alter_info->create_list.elements ||
> + if (table->s->fields != tmp_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 ||
> @@ -5550,7 +5556,7 @@ compare_tables(TABLE *table,
> (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_RECREATE | ALTER_FOREIGN_KEY)) ||
> + (tmp_alter_info.flags & (ALTER_RECREATE | ALTER_FOREIGN_KEY)) ||
> order_num ||
> !table->s->mysql_version ||
> (table->s->frm_version < FRM_VER_TRUE_VARCHAR && varchar))
> @@ -5563,22 +5569,19 @@ compare_tables(TABLE *table,
> Use transformed info to evaluate possibility of fast ALTER TABLE
> but use the preserved field to persist modifications.
> */
> - new_field_it.init(alter_info->create_list);
> tmp_new_field_it.init(tmp_alter_info.create_list);
>
> /*
> Go through fields and check if the original ones are compatible
> with new table.
> */
> - for (f_ptr= table->field, new_field= new_field_it++,
> - tmp_new_field= tmp_new_field_it++;
> +
> +
> + for (f_ptr= table->field, tmp_new_field= tmp_new_field_it++;
> (field= *f_ptr);
> - f_ptr++, new_field= new_field_it++,
> - tmp_new_field= tmp_new_field_it++)
> + f_ptr++, tmp_new_field= tmp_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;
> + DBUG_ASSERT(tmp_new_field->charset != NULL);
>
> /* Check that NULL behavior is same for old and new fields */
> if ((tmp_new_field->flags & NOT_NULL_FLAG) !=
>
>
>
> ------------------------------------------------------------------------
>
>
