#At file:///home/dlenev/src/bzr/mysql-6.1-mil13/ based on revid:dlenev@stripped
2726 Dmitry Lenev 2009-06-30
Milestone 13 "DDL checks and changes: ALTER, CREATE INDEX, DROP INDEX words".
Work in progress.
Moved ALTER TABLE's code handling locking for foreign key names
in LOCK TABLES mode to auxiliary functions.
modified:
mysql-test/r/foreign_key_all_engines_2.result
mysql-test/t/foreign_key_all_engines_2.test
sql/sql_table.cc
=== modified file 'mysql-test/r/foreign_key_all_engines_2.result'
--- a/mysql-test/r/foreign_key_all_engines_2.result 2009-06-25 08:43:04 +0000
+++ b/mysql-test/r/foreign_key_all_engines_2.result 2009-06-30 12:40:10 +0000
@@ -1576,6 +1576,21 @@ t2 CREATE TABLE `t2` (
delete from t1 where a = 1;
ERROR 23000: Foreign key error: constraint 'c': cannot change because foreign key refers to value '1'
#
+# Similar check for alter that tries to replace foreign key.
+alter table t2 drop constraint c,
+add constraint c foreign key (a) references t1 (no_such_column);
+ERROR 42000: Foreign key error: Constraint 'c': No such column 'no_such_column' in parent table
+# Check that metadata was not changed.
+show create table t2;
+Table Create Table
+t2 CREATE TABLE `t2` (
+ `a` int(11) DEFAULT NULL,
+ KEY `c` (`a`),
+ CONSTRAINT `c` FOREIGN KEY (`a`) REFERENCES `t1` (`a`)
+) ENGINE=<engine_type> DEFAULT CHARSET=latin1
+delete from t1 where a = 1;
+ERROR 23000: Foreign key error: constraint 'c': cannot change because foreign key refers to value '1'
+#
# Finally let us see what happens when one uses the same constraint
# name twice.
alter table t2 drop constraint c, drop constraint c;
=== modified file 'mysql-test/t/foreign_key_all_engines_2.test'
--- a/mysql-test/t/foreign_key_all_engines_2.test 2009-06-25 08:43:04 +0000
+++ b/mysql-test/t/foreign_key_all_engines_2.test 2009-06-30 12:40:10 +0000
@@ -1481,6 +1481,16 @@ show create table t2;
--error ER_FK_CHILD_VALUE_EXISTS
delete from t1 where a = 1;
--echo #
+--echo # Similar check for alter that tries to replace foreign key.
+--error ER_FK_PARENT_COLUMN_MISSING
+alter table t2 drop constraint c,
+ add constraint c foreign key (a) references t1 (no_such_column);
+--echo # Check that metadata was not changed.
+--replace_result $engine_type <engine_type>
+show create table t2;
+--error ER_FK_CHILD_VALUE_EXISTS
+delete from t1 where a = 1;
+--echo #
--echo # Finally let us see what happens when one uses the same constraint
--echo # name twice.
--error ER_CANT_DROP_FIELD_OR_KEY
=== modified file 'sql/sql_table.cc'
--- a/sql/sql_table.cc 2009-06-28 06:37:40 +0000
+++ b/sql/sql_table.cc 2009-06-30 12:40:10 +0000
@@ -2389,19 +2389,13 @@ drop_table_check_locks(THD *thd, TABLE_L
LEX_STRING db_name= { table->db, table->db_length };
if (! (fk_name= Foreign_key_name::create(thd->mem_root, db_name,
- fk_c_s->name)))
+ fk_c_s->name)) ||
+ fk_names_locked->push_back(fk_name, thd->mem_root))
return TRUE;
- /*
- TODO/FIXME: Check what happens with list contents in case of error.
- */
-
fk_name->find_ticket(&thd->mdl_context);
DBUG_ASSERT(fk_name->get_mdl_request()->ticket);
-
- if (fk_names_locked->push_back(fk_name, thd->mem_root))
- return TRUE;
}
}
return FALSE;
@@ -8731,6 +8725,267 @@ err:
/**
+ Handle ALTER TABLE's locking for foreign key names under LOCK TABLES
+ by upgrading pre-acquired shared metadata locks on names of foreign
+ keys to be dropped and acquiring exclusive locks for names of foreign
+ keys to be created.
+
+ @param[in] thd Thread context.
+ @param[in] table_list Table list element for table being altered.
+ @param[in] alter_info Alter_info object containing information about
+ foreign keys to be dropped and added.
+ @param[out] fk_names_drop List of names of foreign keys which are going
+ to be dropped, shared metadata locks for which
+ were upgraded to exclusive locks.
+ @param[out] fk_names_add List of names of foreign keys which are going
+ to be added and for which exclusive metadata
+ locks were acquired.
+ @param[out] fk_names_replace List of names of foreign keys which are going
+ to be replaced with new foreign keys with the
+ same names, shared locks for which were
+ upgraded to exclusive locks.
+
+ @note In case of failure caller of this function is still responsible
+ for restoring original state of metadata locks by calling
+ alter_table_restore_fk_name_locks_under_lock_tables() function.
+
+ @retval FALSE Success.
+ @retval TRUE Failure (due to OOM or KILL statement).
+*/
+
+bool
+alter_table_lock_fk_names_under_lock_tables(THD *thd, TABLE_LIST *table_list,
+ Alter_info *alter_info,
+ List<Foreign_key_name> *fk_names_drop,
+ List<Foreign_key_name> *fk_names_add,
+ List<Foreign_key_name> *fk_names_replace)
+{
+ /*
+ For all foreign keys to be dropped find shared metadata locks on their
+ names which were pre-acquired at LOCK TABLES time and upgrade them to
+ exclusive locks. Deadlocks are impossible in this case as connection
+ which holds shared lock on foreign key name also holds TL_WRITE on its
+ child table and thus there can be only one such connection at the moment.
+ */
+ List_iterator<Alter_drop> drop_it(alter_info->drop_list);
+ Alter_drop *drop;
+
+ DBUG_ASSERT(fk_names_drop->is_empty());
+
+ while ((drop= drop_it++))
+ {
+ if (drop->type == Alter_drop::FOREIGN_KEY)
+ {
+ LEX_STRING db_name= { table_list->db, table_list->db_length };
+ LEX_STRING name= { (char *)drop->name, strlen(drop->name) };
+ Foreign_key_name *fk_name;
+ MDL_ticket *mdl_ticket;
+
+ if (! (fk_name= Foreign_key_name::create(thd->mem_root, db_name, name)))
+ return TRUE;
+
+ if (! fk_name->find_ticket(&thd->mdl_context))
+ {
+ /*
+ We have not prelocked this foreign key name at LOCK TABLES time.
+ This means that constraint with such name either does not exist
+ or belongs to table other than one being altered.
+ Don't do anything, this statement will end with an appropriate
+ error in any case.
+ */
+ continue;
+ }
+
+ mdl_ticket= fk_name->get_mdl_request()->ticket;
+
+ if (mdl_ticket->upgrade_shared_lock_to_exclusive())
+ return TRUE;
+
+ if (fk_names_drop->push_back(fk_name, thd->mem_root))
+ {
+ mdl_ticket->downgrade_exclusive_lock();
+ return TRUE;
+ }
+ }
+ }
+
+ /**
+ Now let us try to acquire exclusive metadata locks on names of foreign keys
+ to be created. We avoid deadlocks in this case by using try-lock method
+ instead of one which will wait until conflicting locks will go away.
+ We interpret existence of conflicting lock as existence of foreign key with
+ such name and emit a corresponding error in this case. Indeed, such approach
+ might give false positives if there are other DDL statements which tries to
+ do something with a foreign key with the same name, but since chance of this
+ is fairly low we accept this trade-off.
+
+ Note that the case when new foreign key replaces the old foreign key with
+ the same name has to be handled with special care.
+ */
+
+ List_iterator<Foreign_key_child> fk_it(alter_info->foreign_key_list);
+ Foreign_key_child *fk;
+
+ DBUG_ASSERT(fk_names_add->is_empty() && fk_names_replace->is_empty());
+
+ while ((fk= fk_it++))
+ {
+ Foreign_key_name *fk_name;
+
+ if (! fk->replaces_existing)
+ {
+ MDL_request *mdl_request;
+ MDL_ticket *save_mdl_ticket;
+ LEX_STRING db_name = { table_list->db, table_list->db_length };
+
+ if (! (fk_name= Foreign_key_name::create(thd->mem_root,
+ db_name, fk->name)))
+ return TRUE;
+
+ mdl_request= fk_name->get_mdl_request();
+ mdl_request->set_type(MDL_EXCLUSIVE);
+
+ if (thd->mdl_context.try_acquire_exclusive_lock(mdl_request))
+ return TRUE;
+
+ if (mdl_request->ticket == NULL)
+ {
+ my_error(ER_FK_CONSTRAINT_NAME_DUPLICATE, MYF(0), fk->name.str);
+ return TRUE;
+ }
+
+ /*
+ Remove metadata lock request from the context as memory on
+ which it was allocated would be gone at the end of statement.
+ Store pointer to ticket on stack as removing request from a
+ context clears MDL_request::ticket member.
+ */
+ save_mdl_ticket= mdl_request->ticket;
+ thd->mdl_context.remove_request(mdl_request);
+ mdl_request->ticket= save_mdl_ticket;
+
+ if (fk_names_add->push_back(fk_name, thd->mem_root))
+ {
+ thd->mdl_context.release_lock(save_mdl_ticket);
+ return TRUE;
+ }
+ }
+ else
+ {
+ List_iterator<Foreign_key_name> fk_name_drop_it(*fk_names_drop);
+
+ while ((fk_name= fk_name_drop_it++))
+ if (fk_name->is_equal(table_list->db, fk->name.str))
+ break;
+
+ if (fk_name)
+ {
+ fk_name_drop_it.remove();
+
+ if (fk_names_replace->push_back(fk_name, thd->mem_root))
+ {
+ fk_name->get_mdl_request()->ticket->downgrade_exclusive_lock();
+ return TRUE;
+ }
+ }
+ else
+ {
+ /*
+ Absence of foreign key name in 'fk_name_drop' list when
+ Foreign_key_child::replaces_existing is TRUE means that
+ either this ALTER TABLE statement tries to drop foreign
+ key which does not exist or belongs to different table
+ or that it tries to create two foreign keys with the
+ same name. Since in both these cases this statement will
+ end with an error we can safely continue.
+ */
+ }
+ }
+ }
+
+ return FALSE;
+}
+
+
+/**
+ Restore metadata locks on foreign key names after failing to perform ALTER
+ TABLE under LOCK TABLES (downgrade locks on names which were upgraded and
+ release locks which were acquired).
+
+ @param thd Thread context.
+ @param fk_names_drop List of names of foreign keys which were supposed to
+ be dropped and locks for which should be downgraded.
+ @param fk_names_add List of names of foreign keys which were supposed to
+ be added and locks on which should be released.
+ @param fk_names_replace List of names of foreign keys which were supposed to
+ replaced old foreign keys, locks for which should be
+ downgraded.
+*/
+
+void
+alter_table_restore_fk_name_locks_under_lock_tables(THD *thd,
+ List<Foreign_key_name> &fk_names_drop,
+ List<Foreign_key_name> &fk_names_add,
+ List<Foreign_key_name> &fk_names_replace)
+{
+ List_iterator<Foreign_key_name> fk_drop_it(fk_names_drop);
+ List_iterator<Foreign_key_name> fk_add_it(fk_names_add);
+ List_iterator<Foreign_key_name> fk_replace_it(fk_names_replace);
+ Foreign_key_name *fk_name;
+
+ while ((fk_name= fk_drop_it++))
+ fk_name->get_mdl_request()->ticket->downgrade_exclusive_lock();
+
+ while ((fk_name= fk_add_it++))
+ thd->mdl_context.release_lock(fk_name->get_mdl_request()->ticket);
+
+ while ((fk_name= fk_replace_it++))
+ fk_name->get_mdl_request()->ticket->downgrade_exclusive_lock();
+}
+
+
+/**
+ Adjust metadata locks on foreign key names after performing ALTER TABLE
+ under LOCK TABLES by releasing locks on names of dropped foreign keys
+ and downgrading locks on names of foreign keys which were created.
+
+ @param thd Thread context.
+ @param fk_names_drop List of names of foreign keys which were dropped
+ and therefore locks for which should be released.
+ @param fk_names_add List of names of foreign keys which were added
+ and metadata locks for which should be downgraded
+ from exclusive to shared.
+ @param fk_names_replace List of names of foreign keys which replaced old
+ foreign keys, locks for which should be downgraded.
+*/
+
+void
+alter_table_release_fk_name_locks_under_lock_tables(THD *thd,
+ List<Foreign_key_name> &fk_names_drop,
+ List<Foreign_key_name> &fk_names_add,
+ List<Foreign_key_name> &fk_names_replace)
+{
+ List_iterator<Foreign_key_name> fk_drop_it(fk_names_drop);
+ List_iterator<Foreign_key_name> fk_add_it(fk_names_add);
+ List_iterator<Foreign_key_name> fk_replace_it(fk_names_replace);
+ Foreign_key_name *fk_name;
+
+ /*
+ QQ: Does it makes sense to use release_all_locks_for_name() here ?
+ What is the point behind this function now ?
+ */
+ while ((fk_name= fk_drop_it++))
+ thd->mdl_context.release_lock(fk_name->get_mdl_request()->ticket);
+
+ while ((fk_name= fk_add_it++))
+ fk_name->get_mdl_request()->ticket->downgrade_exclusive_lock();
+
+ while ((fk_name= fk_replace_it++))
+ fk_name->get_mdl_request()->ticket->downgrade_exclusive_lock();
+}
+
+
+/**
Check if table being altered participates in some foreign key as parent
which will be preserved in its new version.
@@ -8820,7 +9075,7 @@ bool mysql_alter_table(THD *thd,char *ne
bool partition_changed= FALSE;
#endif
TABLE_LIST *tab, *parent_tables;
- List<Foreign_key_name> fk_names_add, fk_names_drop;
+ List<Foreign_key_name> fk_names_add, fk_names_drop, fk_names_replace;
DBUG_ENTER("mysql_alter_table");
/*
@@ -9313,97 +9568,13 @@ view_err:
/* We have to do full alter table. */
- /*
- FIXME/TODO: - Consider doing below opt_fk_all_engines only ?
- - Move code to separate function.
- - Check what happens in case of error.
- */
-
- if (thd->locked_tables_mode == LTM_LOCK_TABLES ||
- thd->locked_tables_mode == LTM_PRELOCKED_UNDER_LOCK_TABLES)
- {
- List_iterator<Alter_drop> drop_it(alter_info->drop_list);
- Alter_drop *drop;
-
- while ((drop= drop_it++))
- {
- if (drop->type == Alter_drop::FOREIGN_KEY)
- {
- LEX_STRING db_name= { table_list->db, table_list->db_length };
- LEX_STRING name= { (char *)drop->name, strlen(drop->name) };
- Foreign_key_name *fk_name;
-
- if (! (fk_name= Foreign_key_name::create(thd->mem_root, db_name, name)))
- DBUG_RETURN(TRUE);
-
- if (! fk_name->find_ticket(&thd->mdl_context))
- {
- /*
- We have not prelocked this foreign key name at LOCK TABLES time.
- This means that constraint with such name either does not exist
- or belongs to table other than one being altered.
- Don't do anything, this statement will end with an appropriate
- error in any case.
- */
- continue;
- }
-
- if (fk_name->get_mdl_request()->ticket->upgrade_shared_lock_to_exclusive() ||
- fk_names_drop.push_back(fk_name, thd->mem_root))
- DBUG_RETURN(TRUE);
- }
- }
-
- List_iterator<Foreign_key_child> fk_it(alter_info->foreign_key_list);
- Foreign_key_child *fk;
-
- while ((fk= fk_it++))
- {
- Foreign_key_name *fk_name;
- MDL_request *mdl_request;
- LEX_STRING db_name = { table_list->db, table_list->db_length };
-
- if (! fk->replaces_existing)
- {
-
- if (! (fk_name= Foreign_key_name::create(thd->mem_root,
- db_name, fk->name)) ||
- fk_names_add.push_back(fk_name, thd->mem_root))
- DBUG_RETURN(TRUE);
-
- mdl_request= fk_name->get_mdl_request();
- mdl_request->set_type(MDL_EXCLUSIVE);
-
- if (thd->mdl_context.try_acquire_exclusive_lock(mdl_request))
- DBUG_RETURN(TRUE);
-
- if (mdl_request->ticket == NULL)
- {
- my_error(ER_FK_CONSTRAINT_NAME_DUPLICATE, MYF(0), fk->name.str);
- DBUG_RETURN(TRUE);
- }
- MDL_ticket *ticket= mdl_request->ticket;
- thd->mdl_context.remove_request(mdl_request);
- mdl_request->ticket= ticket;
- }
- else
- {
- List_iterator<Foreign_key_name> fk_name_drop_it(fk_names_drop);
-
- while ((fk_name= fk_name_drop_it++))
- if (fk_name->is_equal(table_list->db, fk->name.str))
- break;
-
- if (fk_name)
- {
- fk_name_drop_it.remove();
-
- if (fk_names_add.push_back(fk_name, thd->mem_root))
- DBUG_RETURN(TRUE);
- }
- }
- }
- }
+ if (opt_fk_all_engines &&
+ (thd->locked_tables_mode == LTM_LOCK_TABLES ||
+ thd->locked_tables_mode == LTM_PRELOCKED_UNDER_LOCK_TABLES) &&
+ alter_table_lock_fk_names_under_lock_tables(thd, table_list, alter_info,
+ &fk_names_drop, &fk_names_add,
+ &fk_names_replace))
+ goto err;
#ifdef WITH_PARTITION_STORAGE_ENGINE
if (prep_alter_part_table(thd, table, alter_info, create_info, old_db_type,
@@ -9900,7 +10071,8 @@ end_online:
thd->mdl_context.remove_request(&target_mdl_request);
}
- if (thd->locked_tables_mode)
+ if (thd->locked_tables_mode == LTM_LOCK_TABLES ||
+ thd->locked_tables_mode == LTM_PRELOCKED_UNDER_LOCK_TABLES)
{
if ((new_name != table_name || new_db != db))
{
@@ -9909,27 +10081,10 @@ end_online:
else
mdl_ticket->downgrade_exclusive_lock();
- /*
- TODO/FIXME: Add comment, consider doing this only for opt_fk_all_engines,
- double check code. Consider what will happen on error path.
- */
- List_iterator<Foreign_key_name> fk_add_it(fk_names_add);
- List_iterator<Foreign_key_name> fk_drop_it(fk_names_drop);
- Foreign_key_name *fk_name;
- MDL_request *mdl_request;
-
- while ((fk_name= fk_add_it++))
- {
- mdl_request= fk_name->get_mdl_request();
- mdl_request->ticket->downgrade_exclusive_lock();
- }
-
- while ((fk_name= fk_drop_it++))
- {
- thd->mdl_context.release_all_locks_for_name(fk_name->get_mdl_request()->
- ticket);
- }
-
+ if (opt_fk_all_engines)
+ alter_table_release_fk_name_locks_under_lock_tables(thd, fk_names_drop,
+ fk_names_add,
+ fk_names_replace);
}
end_temporary:
@@ -9993,6 +10148,12 @@ err:
thd->mdl_context.release_lock(target_mdl_request.ticket);
thd->mdl_context.remove_request(&target_mdl_request);
}
+ if (opt_fk_all_engines &&
+ (thd->locked_tables_mode == LTM_LOCK_TABLES ||
+ thd->locked_tables_mode == LTM_PRELOCKED_UNDER_LOCK_TABLES))
+ alter_table_restore_fk_name_locks_under_lock_tables(thd, fk_names_drop,
+ fk_names_add,
+ fk_names_replace);
DBUG_RETURN(TRUE);
err_with_mdl:
@@ -10009,6 +10170,17 @@ err_with_mdl:
thd->mdl_context.remove_request(&target_mdl_request);
}
thd->mdl_context.release_all_locks_for_name(mdl_ticket);
+
+ /*
+ QQ: May be it makes sense to do something special about FK names in
+ this case instead of restoring original locks?
+ */
+ if (opt_fk_all_engines &&
+ (thd->locked_tables_mode == LTM_LOCK_TABLES ||
+ thd->locked_tables_mode == LTM_PRELOCKED_UNDER_LOCK_TABLES))
+ alter_table_restore_fk_name_locks_under_lock_tables(thd, fk_names_drop,
+ fk_names_add,
+ fk_names_replace);
DBUG_RETURN(TRUE);
}
/* mysql_alter_table */
Attachment: [text/bzr-bundle] bzr/dlenev@mysql.com-20090630124010-r0q3g6g22dhs1qd7.bundle
| Thread |
|---|
| • bzr commit into mysql-6.1-fk branch (dlenev:2726) | Dmitry Lenev | 30 Jun |
