> 2725 Dmitry Lenev 2009-07-31
> WL#148 "Foreign keys".
>
> Milestone 13 "DDL checks and changes: ALTER, CREATE INDEX,
> DROP INDEX words".
As we agreed, please push this into 6.1-fk.
I pushed all small changes, my remaining review items are below.
Let's discuss and address them separately, when you're back
from vacation.
* alter_table_prepare_parent_list() may not always find
the respective share in the Foreign_key_share_list, and it's
not an reported as error.
As a result, the parent list produced by this function may miss
parents of constraints that do not exist.
This only applies to parents of constraints that are
DROPped. Parents of constraints that are CREATEd are always present
and opened successfully.
Preferably, the error should be produced when it is discovered.
If the error for missing constraint is reported in
mysql_prepare_alter_table(), then prepare_parent_list()
should be called after it. Or reporting of the error should
be moved to open_tables(), i.e. before the call to prepare_parent_list().
I don't object (any more) against the goal of having a separate list of
parents, rather than re-using TABLE_LIST::next_global.
However, the way this list is created today is hard
to understand.
* alter_table_check_parent_locks():
It's not obvious why in this function we need to only check
locks of parents of constraints that are being dropped, and
can skip constraints that are being created.
It's because open_tables() in pre-locked mode always
opens all "own" tables, but may leave some not "own" tables
not opened. This explanation hardly makes sense to anyone
except you and me and maybe 1 other engineer :)
In other words, in my view is that this function is a "patch" that
is necessary because implementation of prelocked strategy of
ALTER TABLE (or some other way to parameterize open_tables()
behaviour) is incomplete.
Either we should not have any missing tables at this point,
or every table that was requested to be opened should be missing, and
should be checked.
ALTER TABLE code should not be based on an assumption/invariant
that was put in place for DML implementation.
Under LOCK TABLES, we should not have to write an own function for
every DDL statement to check that the lock type is correct. We
should be able to have a uniform approach to getting the right
tables under LOCK TABLES. In other words, under LOCK TABLES,
I'm looking for something like find_write_locked_table(),
extended to work in the new environment.
* Foreign key shares may still be missing in
context::prepare_alter_table(). Same argument about reporting
an error as early as possible applies: the code would be easier
to understand if it was impossible to have NULL fk_s in
prepare_alter_table().
Alternatively, you could change the code flow to call
prepare_alter_table() after mysql_preapre_alter_table().
*
while ((drop= drop_it++))
{
if (drop->type != Alter_drop::FOREIGN_KEY)
continue;
fk_s_it.rewind();
while ((fk_s= fk_s_it++))
if (! strcmp(drop->name, fk_s->name.str))
break;
This pattern is used multiple times in the code. This indicates
that List<Foreign_key_child> needs to become a container class
that has a method find_by_name().
* prepare_alter_table(): instead of creating new Foreign_key_name
objects, you should find the appropriate object in
LEX::foreign_key_names_to_lock, and use it.
This will be identical to trying to find a corresponding ticket
in lock methods. Once you have only one instance of
Foreign_key_name for each foreign key, assuring it has
a ticket would be simpler.
We suffer (have to perform unnecessary linear searches) from
not having a link from Alter_drop to foreign key share and
foreign key MDL ticket. We could store a pointer to foreign key
share in foreign key MDL ticket and access it with
get_cached_object().
* I can understand your desire to keep mysql_prepare_create_table()
unaware of DDL rcontext.
However, calling fk_ctx.add_fks_to_parent_descriptions()
right in front of install_new_frms() should not be done either: you
should call this method when you call mysql_prepare_create_table() or
mysql_prepare_alter_table() for the subject table.
* upgrade_mdl_for_altered_and_parent_tables():
instead of calling wait_while_table_is_used() for a single
table, we need a mechanism for lock upgrade for a list of tables,
implemented in MDL. Waiting while holding exclusive metadata
locks can leads to deadlocks (yes I admit I wasn't able to find
a case right away).
* *_under_lock_tables(): this functionality
You should not need to maintain separate on-stack lists for
foreign key names in case of LOCK TABLES - they should be part
of the prelocking context.
LOCK TABLES specific locking should be moved to
upgrade/downgrade functions, at least invoked from there. In
mysql_alter_table() code, places where we process locks under
LOCK TABLES should not be different from the standard mode.
These functions could still be kept around, to encapsulate lock
tables-specific functionality, but there should be little
traces of LOCK TABLES mode in ALTER TABLE code.
* remove alter_table_adjust_tables()
* add a comment giving an overview of the locking algorithm used
by ALTER, DROP and CREATE table with foreign keys.
* consider changing fk_add_table_to_list() to add only unique
instances, and merging it with drop_table_add_parent_tables()
Changes to be done in 5.4:
* Extension of the prelocking set with TABLE_LISTs for views
tables should be moved to prelocking_strategy->handle_view().
* sroutines_hash_entry needs to use MDL_key and a member
to reflect the requested lock type (looks like it should contain
MDL request?)
TYPE_ENUM_FOREIGN_KEY_NAME and TYPE_ENUM_FOREIGN_KEY is abuse
of the type system. We need to kill these types altogether,
and use MDL types instead.
* lex->foreign_key_names_to_lock must be gone; open_tables()
should use sroutines list to prelock keys.
We need a generic open_tables() implementation that has
no special hooks for different SQL statements.
* open_tables() needs to be rewritten to call open_routines
only in one place (or perhaps two: one to open those routines
that need to be locked exclusively, and another time for shared
locks).
Right now there are some implicit assumptions: e.g. that
foreign keys and stored functions are never present in the
list together.
* rename all methods that extend the prelocking set (triggers,
views, etc) to extend_prelocking_set(). Perhaps it is a
candidate for a virtual interface.
* move the resetting of MDL_request::type to reinit_stmt_before_use
from remove_all_requests().
Let's discuss these points when you're back.
They are all medium or large, but I consider the list itself
to be short (especially for 12k patch).
Thank you very much for working on this, this code is difficult.
--
