maria-developers team mailing list archive

[Alexey Botchkov <holyfoot@xxxxxxxxxxxx>]

30.07.2012 18:27, Sergei Golubchik wrote:
> 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?
That's right.

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

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

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

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().


And one more thing.
It's easy to imagine an 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()

Using the separate function like i did it's easy to notice that the 
current ALTER is NOOP and return at once.


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

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.

Best regards.
HF