Hi, Holyfoot! On Jul 08, holyfoot@askmonty.org wrote:
At file:///home/hf/wmar/mdev-318/
------------------------------------------------------------ revno: 3552 revision-id: holyfoot@askmonty.org-20120708152354-d7iu7yoqo3cohidz parent: sanja@montyprogram.com-20120626184334-ptpg39ptq3t79dg5 committer: Alexey Botchkov <holyfoot@askmonty.org> branch nick: mdev-318 timestamp: Sun 2012-07-08 20:23:54 +0500 message: MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE (MWL #252). ALTER TABLE syntax modified and related implementations added to allow: ALTER TABLE ADD/DROP COLUMN ALTER TABLE ADD/DROP INDEX ALTER TABLE ADD/DROP FOREIGN KEY ALTER TABLE ADD/DROP PARTITION ALTER TABLE CHANGE COLUMN ALTER TABLE MODIFY COLUMN
Please, find my comments below:
=== modified file 'mysql-test/r/alter_table.result' --- mysql-test/r/alter_table.result 2011-11-21 17:13:14 +0000 +++ mysql-test/r/alter_table.result 2012-07-11 14:24:32 +0000 @@ -1282,3 +1282,30 @@ # Clean-up. drop table t1; End of 5.1 tests +CREATE TABLE t1 ( +id INT(11) NOT NULL, +x_param INT(11) DEFAULT NULL, +PRIMARY KEY (id), +KEY x_param (x_param) +); +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS id INT, +ADD COLUMN IF NOT EXISTS lol INT AFTER id; +Warnings: +Note 1060 Duplicate column name 'id' +ALTER TABLE t1 ADD COLUMN IF NOT EXISTS lol INT AFTER id; +Warnings: +Note 1060 Duplicate column name 'lol' +ALTER TABLE t1 DROP COLUMN IF EXISTS lol; +ALTER TABLE t1 DROP COLUMN IF EXISTS lol; +Warnings: +Note 1091 Can't DROP 'lol'; check that column/key exists +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param); +Warnings: +Note 1061 Duplicate key name 'x_param' +ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param);
typo? you test ADD KEY IF NOT EXISTS twice.
+Warnings: +Note 1061 Duplicate key name 'x_param' +ALTER TABLE t1 MODIFY IF EXISTS lol INT; +Warnings: +Note 1054 Unknown column 'lol' in 't1' +DROP TABLE t1; === modified file 'sql/field.cc' --- sql/field.cc 2012-03-13 14:38:43 +0000 +++ sql/field.cc 2012-07-11 14:28:40 +0000 @@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en
First: please, see the last section in http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb... and do as it suggests I had to apply your patch locally and re-run bzr diff to get the function names in the diff :(
(maybe_null ? FIELDFLAG_MAYBE_NULL : 0) | (is_unsigned ? 0 : FIELDFLAG_DECIMAL)); vcol_info= 0; + create_if_not_exists= FALSE; stored_in_db= TRUE; }
=== modified file 'sql/sql_lex.h' --- sql/sql_lex.h 2011-12-11 16:39:33 +0000 +++ sql/sql_lex.h 2012-07-11 14:28:40 +0000 @@ -1816,6 +1816,7 @@ typedef struct st_lex : public Query_tab uint16 create_view_algorithm; 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
bool autocommit; bool verbose, no_write_to_binlog;
=== modified file 'sql/sql_yacc.yy' --- sql/sql_yacc.yy 2012-06-10 09:53:06 +0000 +++ sql/sql_yacc.yy 2012-07-11 14:28:40 +0000 @@ -4656,8 +4659,16 @@ table_option: ;
opt_if_not_exists: - /* empty */ { $$= 0; } - | IF not EXISTS { $$=HA_LEX_CREATE_IF_NOT_EXISTS; } + /* empty */ + { + Lex->check_exists= FALSE;
Let's initialize check_exists either before the grammar is parsed (like in the add_create_index_prepare and in the create rule) of in the grammar (here, in the opt_if_not_exists). But don't do it twice.
+ $$= 0; + } + | IF not EXISTS + { + Lex->check_exists= TRUE; + $$=HA_LEX_CREATE_IF_NOT_EXISTS; + } ;
opt_create_table_options: @@ -5879,6 +5891,18 @@ opt_ident: | field_ident { $$=$1.str; } ;
+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?
opt_component: /* empty */ { $$= null_lex_str; } | '.' ident { $$= $2; } @@ -6455,6 +6482,11 @@ opt_column: | COLUMN_SYM {} ;
+opt_column_if_exists: + opt_column if_exists + {} + ; +
same as above - why a special rule?
opt_ignore: /* empty */ { Lex->ignore= 0;} | IGNORE_SYM { Lex->ignore= 1;} @@ -10096,8 +10128,16 @@ table_alias_ref: ;
if_exists:
please, rename this old rule to opt_if_exists
- /* empty */ { $$= 0; } - | IF EXISTS { $$= 1; } + /* empty */ + { + Lex->check_exists= FALSE; + $$= 0; + } + | IF EXISTS + { + Lex->check_exists= TRUE; + $$= 1; + } ;
opt_temporary:
you didn't change CREATE INDEX and DROP INDEX, did you? and ALTER TABLE DROP PRIMARY KEY isn't covered either
=== 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.
+ thd Thread object. + table The altered table. + alter_info List of columns and indexes to create + + DESCRIPTION + Looks for the IF [NOT] EXISTS options, checks the states and remove items + from the list if existing found. + + RETURN VALUES + NONE +*/ + +static void +handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) +{ + Field **f_ptr;
Regards, Sergei