List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:July 31 2009 4:53pm
Subject:Re: bzr commit into mysql-6.1-fk branch (dlenev:2725) WL#148
View as plain text  
>  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.

-- 

Thread
bzr commit into mysql-6.1-fk branch (dlenev:2725) WL#148Dmitry Lenev31 Jul
  • Re: bzr commit into mysql-6.1-fk branch (dlenev:2725) WL#148Konstantin Osipov31 Jul