maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Alexey!

On Aug 08, Alexey Botchkov wrote:
> > === 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()
> > 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().
> 
> That's not enough. You also should grep for ER_SAME_NAME_PARTITION,
> ER_DROP_PARTITION_NON_EXISTENT, ER_CANT_DROP_FIELD_OR_KEY,
> ER_BAD_FIELD_ERROR

Okay. Only one occurence for ER_SAME_NAME_PARTITION and
ER_CANT_DROP_FIELD_OR_KEY.
Many for ER_BAD_FIELD_ERROR, but only one is relevant to this task.

Only ER_DROP_PARTITION_NON_EXISTENT can be issued in more than one
place - but partitining code is generally pretty awful, so it's not
surprising.

But aside of ER_DROP_PARTITION_NON_EXISTENT, every other error happens
in only one place. As far as I can see.

> >> 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?
> 
> Yes. Still i can imagine one setting that value and we're having 
> mysterious bugs as a result.

because only the parser can set this value - if it's set elsewhere
this is a bug on itself. and it'd better be fixed, not masked by
ignoring the symptom.

> >> 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.
> 
> Not really.
> For fields it works like that:
> mysql_alter_table()
>      calls mysql_prepare_alter_table()
>           there the new_alter_info is formed from all the old fields
>           that weren't dropped and all the added fields. No duplicated
>           fields checks yet.
>       then it calls compare_tables()
>             which calls mysql_prepare_create_table()
>                      which finally discovers the duplicated fields.
>                      The code to remove the field with the 'if_exists'
>                      mark from the list look ugly there. Which is
>                      worse, there it's no use to remove something from
>                      the list there at all as it gets only a temporary
>                      copy of the Alter_info.  Later in the code we
>                      call the same mysql_prepare_create_table() with
>                      the real data.

I see. That's right. I've checked the history in bzr to see why
compare_tables() works with a temporary copy of the Alter_info.

The reason, as far as I can see, was that it shouldn't do permanent
changes to the LEX structure, as it breaks stored routines, the
statement can no longer be reexecuted correctly.

If we'll go with your approach, it must work in this use case. You are
changing the original LEX structure, don't you?  Could you please extend
the test cases to cover that? See the changeset I'm referring to above
(compare_tables(), etc) for these use cases.

> For partitions code is not nice for removing something from the list at all.
> And we have to provide the 'check_exists' flag to the remote functions
> like partition_info::check_partition_info().

yuck. that's bad and buggy piece of code. feel free to remove the check
for duplicate names from check_partition_info and move it elsewhere.

Test case:

CREATE TABLE t1 ( f_int1 INTEGER, f_int2 INTEGER, f_char1 CHAR(20),
f_char2 CHAR(20), f_charbig VARCHAR(1000)) PARTITION BY RANGE(f_int1)
SUBPARTITION BY KEY(f_int1,f_int1) ( PARTITION part1 VALUES LESS THAN
(1000) (SUBPARTITION subpart11, SUBPARTITION subpart1));

it should be an error, because of "KEY(f_int1,f_int1)"

> And one more thing.
> It's easy to imagine a user that likes to do the ALTER IF EXISTS 
> queries often.
> 
> But even if the 'new_field' exists already, ADD FIELD IF EXISTS 
> new_field still does a lot.
> The list of calls is:
>       mysql_create_table_no_lock()  - creates the .frm file with the
>                                       temporary name
>       mysql_rename_table()
>       mysql_rename_table()   once more
>       quick_rm_table()  - removes the old .frm
>       query_cache_invalidate()
>       reopen_table()

Hmm, I don't see it. If the field exists, mysql_alter_table will notice
it in compare_tables() - that is, before mysql_create_table_no_lock().

> > 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.
> 
> So i see two options - seriously revise the way mysql_alter_table works or
> keep the EXISTS handling in the separate function.
> BTW i don't feel the separate function is that bad. I don't duplicate that
> much code there besides the loops. But we do such loops in many places 
> anyway.

Loops aren't the problem, the problem is the unmaintainable spaghetti
code. We have a lot of it, and we should improve the situation, not make
it worse.

So, if possible, I'd encourage you to look at how mysql_alter_table
checks its input and and improve that.

Regards,
Sergei