MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Gleb Shchepa Date:July 29 2009 10:34am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (azundris:2973) Bug#43508
View as plain text  
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) !=
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 

Thread
bzr commit into mysql-5.1-bugteam branch (azundris:2973) Bug#43508Tatiana A. Nurnberg24 Jun
  • Re: bzr commit into mysql-5.1-bugteam branch (azundris:2973) Bug#43508Gleb Shchepa29 Jul
    • Re: bzr commit into mysql-5.1-bugteam branch (azundris:2973) Bug#43508Tatiana Azundris9 Oct