← Back to team overview

maria-developers team mailing list archive

Re: MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE

 

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



Follow ups

References