maria-developers team mailing list archive

[Alexey Botchkov <holyfoot@xxxxxxxxxxxx>]

11.07.2012 20:14, Sergei Golubchik wrote:
> +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.

Yes, but not this line. The 'KEY x_param' wasn't supposed to be created 
with the CREATE TABLE.

>     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

Seems to work so far...

> > +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?

I'm afraid if i didn't create this rule, i had to add this code to 4 
cases of the 'key_def:'
And i'd also had to modify more lines, as numbers of parameters after the
opt_if_not_exists would increase.

>   
> +opt_column_if_exists:
> +        opt_column if_exists
> +        {}
> +        ;
> +
> same as above - why a special rule?

Here the sole reason is to avoid modifying the code with the increased 
parameter's numbers.
Ok, i can it's not enough to add a separate rule, going to get rid of 
this one.

> > === 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.

Naturally i started doing it that way initially.
Then i realized the code looks rather ugly. These checks are scattered 
around the code and
parts of the code with these checks are used not only for ALTER TABLE, 
but for CREATE and other internal needs.
There we need to be sure these check_if_exist flags aren't set.
And the loops should be done in a different way in these places so we 
can remove items from the list in the process.
Thus i decided to do that in the separate function to be called from the 
mysql_alter_table exclusively.

In my opinion, the fuction won't affect the performance of the existing 
functions at all. And it's easy
to understand what and why it does, so the code duplication will produce 
the more error-prone code here.

Well, did i convince you the separate function is nicer?

Best regards.
HF