maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Alexey!

First: You didn't include all my comments in your reply.
Should I assume that all omitted comments you simply agree with and
you will change the patch accordingly?

On Jul 16, Alexey Botchkov wrote:
> 11.07.2012 20:14, Sergei Golubchik wrote:
> 
> >     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...

Yes. I mean, LEX::drop_if_exists is an old flag that remembers whether
IF EXISTS was used in DROP TABLE. While check_exists is a new flag that
remembers whether IF EXISTS or IF NOT EXIST was used in ALTER TABLE (and
possibly in CREATE INDEX, etc).

So, the old flag is only used when dropping a table, and the new one -
when dropping or adding a column, partition, or an index.

I suggest to remove drop_if_exists flag completely and use check_exists
also for DROP TABLE.

> >> +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:'

Ah, indeed. key_def is used both in CREATE and ALTER, and you need your
special check to allow the new syntax only in ALTER.

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

Yes, please.

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

I didn't spend on this much time, but grepping for
ER_DUP_FIELDNAME and ER_DUP_KEYNAME (which, I suppose, should show all
checks), didn't show all that many places. Only in sql_table.cc, and
only in mysql_prepare_create_table().

> There we need to be sure these check_if_exist flags aren't set.

Yes, but because it's not set by default, it should be always unset,
unless you set it explicitly in sql_yacc.yy, right?

> And the loops should be done in a different way in these places so we 
> can remove items from the list in the process.

It is already supported by the current code - both for fields and keys.

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

I'm sorry. Not quite.
If the current code is structured badly, it does not mean that we should
add a special function with duplicate checks for every new task.

If you think the current code is ugly, please, do a little refactoring
and move these checks to separate functions, that you can easily extend
for your purposes.

But, really, I don't see even this being necessary, I think it would be
easy to add checks to the existing code as it is. But I may be wrong, of
course.

Regards,
Sergei