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