maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jan!

A question and a suggestion, see below

On Jul 27, jan wrote:
> revision-id: 4c26b71c77d58fbeb3192e2142ef66efe5f122dd (mariadb-10.0.31-43-g4c26b71c77d)
> parent(s): a8c1817846eb9f3fd8738e80db87cff642b9a78b
> author: Jan Lindström
> committer: Jan Lindström
> timestamp: 2017-07-27 13:17:13 +0300
> message:
> 
> MDEV-8960: Can't refer the same column twice in one ALTER TABLE
> 
> Problem was that if column was created in alter table when
> it was refered again it was not tried to find from list
> of current columns.
> 
> mysql_prepare_alter_table:
>   There is two cases
>     (1) If alter table adds a new column and then later alter
>         changes the field definition, there was no check from
> 	list of new columns, instead an incorrect error was given.
>     (2) If alter table adds a new column and then later alter
>         changes the default, there was no check from list of
> 	new columns, instead an incorrect error was given.
> 
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 2eff8fd5e2f..3ba3ec6847e 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -7503,9 +7503,25 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
>    {
>      if (def->change && ! def->field)
>      {
> -      my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change,
> -               table->s->table_name.str);
> -      goto err;
> +      /*
> +        Check if there is modify for newly added field.
> +      */
> +      Create_field *find;
> +      find_it.rewind();
> +      while((find=find_it++))
> +      {
> +        if (!my_strcasecmp(system_charset_info,find->field_name, def->field_name))
> +          break;
> +      }
> +
> +      if (find && !find->field)

Can it be here that find->field != NULL?
I don't see how. But if yes, the ER_BAD_FIELD_ERROR looks rather weird
in that case. If not - simple if (find) would be less confusing.

> +        find_it.remove();
> +      else
> +      {
> +        my_error(ER_BAD_FIELD_ERROR, MYF(0), def->change,
> +                 table->s->table_name.str);
> +        goto err;
> +      }
>      }
>      /*
>        Check that the DATE/DATETIME not null field we are going to add is
> @@ -7571,6 +7587,29 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
>          find_it.after(def);			// Put column after this
>        }
>      }
> +    /*
> +      Check if there is alter for newly added field.
> +    */
> +    alter_it.rewind();
> +    Alter_column *alter;
> +    while ((alter=alter_it++))
> +    {
> +      if (!my_strcasecmp(system_charset_info,def->field_name, alter->name))
> +        break;
> +    }
> +    if (alter)
> +    {
> +      if (def->sql_type == MYSQL_TYPE_BLOB)
> +      {
> +        my_error(ER_BLOB_CANT_HAVE_DEFAULT, MYF(0), def->change);
> +        goto err;
> +      }
> +      if ((def->def=alter->def))              // Use new default
> +        def->flags&= ~NO_DEFAULT_VALUE_FLAG;
> +      else
> +        def->flags|= NO_DEFAULT_VALUE_FLAG;
> +      alter_it.remove();
> +    }

Please, move this into a function. Don't just copy 20 lines around.
For example

     if (apply_alter_column(alter_it, def, field->field_name))
       goto err;

>    }
>    if (alter_info->alter_list.elements)
>    {

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx