maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Holyfoot!

On Jul 08, holyfoot@xxxxxxxxxxxx wrote:
> At file:///home/hf/wmar/mdev-318/
> 
> ------------------------------------------------------------
> revno: 3552
> revision-id: holyfoot@xxxxxxxxxxxx-20120708152354-d7iu7yoqo3cohidz
> parent: sanja@xxxxxxxxxxxxxxxx-20120626184334-ptpg39ptq3t79dg5
> committer: Alexey Botchkov <holyfoot@xxxxxxxxxxxx>
> branch nick: mdev-318
> timestamp: Sun 2012-07-08 20:23:54 +0500
> message:
>   MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE (MWL #252).
>   ALTER TABLE syntax modified and related implementations added to allow:
>       ALTER TABLE ADD/DROP COLUMN
>       ALTER TABLE ADD/DROP INDEX
>       ALTER TABLE ADD/DROP FOREIGN KEY
>       ALTER TABLE ADD/DROP PARTITION
>       ALTER TABLE CHANGE COLUMN
>       ALTER TABLE MODIFY COLUMN

Please, find my comments below:

> === modified file 'mysql-test/r/alter_table.result'
> --- mysql-test/r/alter_table.result	2011-11-21 17:13:14 +0000
> +++ mysql-test/r/alter_table.result	2012-07-11 14:24:32 +0000
> @@ -1282,3 +1282,30 @@
>  # Clean-up.
>  drop table t1;
>  End of 5.1 tests
> +CREATE TABLE t1 (
> +id INT(11) NOT NULL,
> +x_param INT(11) DEFAULT NULL,
> +PRIMARY KEY (id),
> +KEY x_param (x_param)
> +);
> +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS id INT,
> +ADD COLUMN IF NOT EXISTS lol INT AFTER id;
> +Warnings:
> +Note	1060	Duplicate column name 'id'
> +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS lol INT AFTER id;
> +Warnings:
> +Note	1060	Duplicate column name 'lol'
> +ALTER TABLE t1 DROP COLUMN IF EXISTS lol;
> +ALTER TABLE t1 DROP COLUMN IF EXISTS lol;
> +Warnings:
> +Note	1091	Can't DROP 'lol'; check that column/key exists
> +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param);
> +Warnings:
> +Note	1061	Duplicate key name 'x_param'
> +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param);

typo? you test ADD KEY IF NOT EXISTS twice.

> +Warnings:
> +Note	1061	Duplicate key name 'x_param'
> +ALTER TABLE t1 MODIFY IF EXISTS lol INT;
> +Warnings:
> +Note	1054	Unknown column 'lol' in 't1'
> +DROP TABLE t1;
> === modified file 'sql/field.cc'
> --- sql/field.cc	2012-03-13 14:38:43 +0000
> +++ sql/field.cc	2012-07-11 14:28:40 +0000
> @@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en

First: please, see the last section in
http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb/
and do as it suggests

I had to apply your patch locally and re-run bzr diff
to get the function names in the diff :(

>                (maybe_null ? FIELDFLAG_MAYBE_NULL : 0) |
>                (is_unsigned ? 0 : FIELDFLAG_DECIMAL));
>    vcol_info= 0;
> +  create_if_not_exists= FALSE;
>    stored_in_db= TRUE;
>  }
>  
> === modified file 'sql/sql_lex.h'
> --- sql/sql_lex.h	2011-12-11 16:39:33 +0000
> +++ sql/sql_lex.h	2012-07-11 14:28:40 +0000
> @@ -1816,6 +1816,7 @@ typedef struct st_lex : public Query_tab
>    uint16 create_view_algorithm;
>    uint8 create_view_check;
>    bool drop_if_exists, drop_temporary, local_file, one_shot_set;
> +  bool check_exists; /* Enabled if the IF [NOT] EXISTS specified for ALTER */

I think, you can remove drop_if_exists and use check_exists instead

>    bool autocommit;
>    bool verbose, no_write_to_binlog;
>  
> === modified file 'sql/sql_yacc.yy'
> --- sql/sql_yacc.yy	2012-06-10 09:53:06 +0000
> +++ sql/sql_yacc.yy	2012-07-11 14:28:40 +0000
> @@ -4656,8 +4659,16 @@ table_option:
>          ;
>  
>  opt_if_not_exists:
> -          /* empty */ { $$= 0; }
> -        | IF not EXISTS { $$=HA_LEX_CREATE_IF_NOT_EXISTS; }
> +          /* empty */
> +          {
> +            Lex->check_exists= FALSE;

Let's initialize check_exists either before the grammar is parsed
(like in the add_create_index_prepare and in the create rule)
of in the grammar (here, in the opt_if_not_exists).
But don't do it twice.

> +            $$= 0;
> +          }
> +        | IF not EXISTS
> +          {
> +            Lex->check_exists= TRUE;
> +            $$=HA_LEX_CREATE_IF_NOT_EXISTS;
> +          }
>          ;
>  
>  opt_create_table_options:
> @@ -5879,6 +5891,18 @@ opt_ident:
>          | field_ident { $$=$1.str; }
>          ;
>  
> +opt_if_not_exists_ident:
> +        opt_if_not_exists opt_ident
> +        {
> +          LEX *lex= Lex;
> +          if (lex->check_exists && lex->sql_command != SQLCOM_ALTER_TABLE)
> +          {
> +            my_parse_error(ER(ER_SYNTAX_ERROR));
> +            MYSQL_YYABORT;
> +          }
> +          $$= $2;
> +        };
> +

why did you create opt_if_not_exists_ident rule, instead of adding
opt_if_not_exists before ident in the alter table grammar?

>  opt_component:
>            /* empty */    { $$= null_lex_str; }
>          | '.' ident      { $$= $2; }
> @@ -6455,6 +6482,11 @@ opt_column:
>          | COLUMN_SYM {}
>          ;
>  
> +opt_column_if_exists:
> +        opt_column if_exists
> +        {}
> +        ;
> +

same as above - why a special rule?

>  opt_ignore:
>            /* empty */ { Lex->ignore= 0;}
>          | IGNORE_SYM { Lex->ignore= 1;}
> @@ -10096,8 +10128,16 @@ table_alias_ref:
>          ;
>  
>  if_exists:

please, rename this old rule to opt_if_exists

> -          /* empty */ { $$= 0; }
> -        | IF EXISTS { $$= 1; }
> +          /* empty */
> +        {
> +          Lex->check_exists= FALSE;
> +          $$= 0;
> +        }
> +        | IF EXISTS
> +        {
> +          Lex->check_exists= TRUE;
> +          $$= 1;
> +        }
>          ;
>  
>  opt_temporary:

you didn't change CREATE INDEX and DROP INDEX, did you?
and ALTER TABLE DROP PRIMARY KEY isn't covered either

> === modified file 'sql/sql_table.cc'
> --- sql/sql_table.cc	2012-04-05 21:07:18 +0000
> +++ sql/sql_table.cc	2012-07-11 14:28:40 +0000
> @@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
>  
>  
>  /*
> +  Preparation for table creation
> +
> +  SYNOPSIS
> +    handle_if_exists_option()

Why did you do that in a separate function? You have basically duplicated
all checks. normal alter table code checks for duplicate columns/key names
and for non-existant columns/keys for DROP.

this code is still in place after your patch. so, basically,
you'll check for duplicates and non-existant names twice.
what's the point of that?

I'd expect you to take check_exists flag into account
in the existing checking code and convert errors into warnings
as appropriate.

> +      thd                       Thread object.
> +      table                     The altered table.
> +      alter_info                List of columns and indexes to create
> +
> +  DESCRIPTION
> +    Looks for the IF [NOT] EXISTS options, checks the states and remove items
> +    from the list if existing found.
> +
> +  RETURN VALUES
> +    NONE
> +*/
> +
> +static void
> +handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info)
> +{
> +  Field **f_ptr;

Regards,
Sergei