Re: bk commit into 5.1 tree (istruewing:1.2595) BUG#26379

[Konstantin Osipov]

* Ingo Struewing <ingo@stripped> [07/11/02 19:43]:

> ChangeSet@stripped, 2007-11-02 17:28:19+01:00, istruewing@stripped +34 -0
>   Bug#26379 - Combination of FLUSH TABLE and REPAIR TABLE
>               corrupts a MERGE table
>   Bug 26867 - LOCK TABLES + REPAIR + merge table result in
>               memory/cpu hogging
>   Bug 26377 - Deadlock with MERGE and FLUSH TABLE
>   Bug 25038 - Waiting TRUNCATE
>   Bug 25700 - merge base tables get corrupted by
>               optimize/analyze/repair table
>   Bug 30275 - Merge tables: flush tables or unlock tables
>               causes server to crash
>   Bug 19627 - temporary merge table locking
>   Bug 27660 - Falcon: merge table possible
>   Bug 30273 - merge tables: Can't lock file (errno: 155)

Hello Ingo,

Thank you so much for working on this.

The patch is OK to push, please see my review comments below.

Being the second reviewer, I'm not in the position of making
architecture comments. 

Some less major comments still exist.

As for the architecture issues, I think the work on merge tables
is not finished.

In particular I believe that: 
 * we need to avoid the scheme with mysql_lock_abort forwarding and
   counting locks for merge children. Instead we should call
   full-blown lock_tables for merge children,
 * we should attach/detach merge children at the beginning of a
   statement (sub-statement), not when the tables are locked. That
   will solve a large chunk of problems when the parent becomes
   invalid because of the DDL on the child under lock tables.
   That should also unify attach/detach of temporary and base
   tables.
   A lot of code from the current patch could be removed after
   that.
 * attach/detach should happen inside merge handler methods, not
   in explicit methods in sql/, like now (detach_merge_children). 
 * no merge parent/child links should exist in TABLE objects.
   Without abort forwarding they should be unnecessary. Links
   in TABLE_LIST objects should stay.

We should discuss these changes, create a worklog entry for them and
implement it separately, perhaps in 5.2 only.

I shall appeal to Calvin, Joro, and Jeffrey for your exclusion
from the bug team for the remaining of November so that these
changes can happen soon.

Please see my other comments inline.

> diff -Nrup a/sql/slave.cc b/sql/slave.cc
> --- a/sql/slave.cc	2007-08-29 23:28:34 +02:00
> +++ b/sql/slave.cc	2007-11-02 17:28:15 +01:00
> @@ -1013,8 +1013,8 @@ static int create_table_from_dump(THD* t
>      goto err;                   // mysql_parse took care of the error send
>  
>    thd->proc_info = "Opening master dump table";
> -  tables.lock_type = TL_WRITE;
> -  if (!open_ltable(thd, &tables, TL_WRITE, 0))
> +  tables.table= NULL; /* was set by mysql_rm_table(). */
> +  if (!open_n_lock_single_table(thd, &tables, TL_WRITE))
>    {
>      sql_print_error("create_table_from_dump: could not open created table");
>      goto err;
> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc	2007-10-19 22:03:16 +02:00
> +++ b/sql/sql_base.cc	2007-11-02 17:28:15 +01:00
> @@ -683,6 +683,9 @@ TABLE_SHARE *get_cached_table_share(cons
>      to open the table
>  
>      thd->killed will be set if we run out of memory
> +
> +    If closing a MERGE child, the calling fuction has to take care for

fuction -> function

> @@ -834,6 +847,11 @@ static void free_cache_entry(TABLE *tabl
>  {
>    DBUG_ENTER("free_cache_entry");
>  
> +  /* Assert that MERGE children are not attached before final close. */
> +  DBUG_ASSERT((!table->child_l && !table->parent) ||
> +              (table->child_l && !table->children_attached) ||
> +              (table->parent &&
> !table->parent->children_attached));

I believe that any check that is more than one line should be
moved to a function. 

I suggest to make a TABLE method from this function, and assert
for:

  DBUG_ASSERT(! table->has_attached_children())

>    intern_close_table(table);
>    if (!table->in_use)
>    {
> @@ -900,6 +918,54 @@ bool close_cached_tables(THD *thd, bool 
>        pthread_mutex_lock(&oldest_unused_share->mutex);
>        VOID(hash_delete(&table_def_cache, (uchar*) oldest_unused_share));
>      }
> +    DBUG_PRINT("tcache", ("incremented global refresh_version to: %lu",
> +                          refresh_version));
> +    if (if_wait_for_refresh)
> +    {
> +      /*
> +        Other threads could wait in a loop in open_and_lock_tables(),
> +        trying to lock one or more of our tables.
> +
> +        If they wait for the locks in thr_multi_lock(), their lock
> +        request is aborted. They loop in open_and_lock_tables() and
> +        enter open_table(). Here they notice the table is refreshed and
> +        wait for COND_refresh. Then they loop again in
> +        open_and_lock_tables() and this time open_table() succeeds. At
> +        this moment, if we (the FLUSH TABLES thread) are scheduled and
> +        on another FLUSH TABLES enter close_cached_tables(), they could
> +        awake while we sleep below, waiting for others threads (us) to
> +        close their open tables. If this happens, the other threads
> +        would find the tables unlocked. They would get the locks, one
> +        after the other, and could do their destructive work. This is an
> +        issue if we have LOCK TABLES in effect.
> +
> +        The problem is that the other threads passed all checks in
> +        open_table() before we refresh the table.
> +
> +        The fix for this problem is to set some_tables_deleted for all
> +        threads with open tables. These threads can still get their
> +        locks, but will immediately release them again after checking
> +        this variable. They will then loop in open_and_lock_tables()
> +        again. There they will wait until we update all tables version
> +        below.
> +
> +        Setting some_tables_deleted is done by remove_table_from_cache()
> +        in the other branch.
> +
> +        In other words (reviewer suggestion): You need this setting of
> +        some_tables_deleted for the case when table was opened and all
> +        related checks were passed before incrementing refresh_version
> +        (which you already have) but attempt to lock the table happened
> +        after the call to close_old_data_files() i.e. after removal of
> +        current thread locks.
> +      */
> +      for (uint idx=0 ; idx < open_cache.records ; idx++)
> +      {
> +        TABLE *table=(TABLE*) hash_element(&open_cache,idx);
> +        if (table->in_use)
> +          table->in_use->some_tables_deleted= 1;
> +      }
> +    }

Sigh. OK. 

>  void close_thread_tables(THD *thd, bool lock_in_use, bool skip_derived)
>  {
> +  TABLE *table;
>    bool found_old_table;
>    prelocked_mode_type prelocked_mode= thd->prelocked_mode;
>    DBUG_ENTER("close_thread_tables");
>  
> +#ifdef EXTRA_DEBUG
> +  DBUG_PRINT("tcache", ("open tables:"));
> +  for (table= thd->open_tables; table; table= table->next)
> +    DBUG_PRINT("tcache", ("table: '%s'.'%s' 0x%lx", table->s->db.str,
> +                          table->s->table_name.str, (long) table));
> +#endif
> +
>    /*
>      We are assuming here that thd->derived_tables contains ONLY derived
>      tables for this substatement. i.e. instead of approach which uses
> @@ -1133,7 +1207,7 @@ void close_thread_tables(THD *thd, bool 
>    */
>    if (thd->derived_tables && !skip_derived)
>    {
> -    TABLE *table, *next;
> +    TABLE *next;
>      /*
>        Close all derived tables generated in queries like
>        SELECT * FROM (SELECT * FROM t1)
> @@ -1154,6 +1228,19 @@ void close_thread_tables(THD *thd, bool 
>      mark_used_tables_as_free_for_reuse(thd, thd->temporary_tables);
>    }

<cut>

> @@ -1154,6 +1228,19 @@ void close_thread_tables(THD *thd, bool
>      mark_used_tables_as_free_for_reuse(thd, thd->temporary_tables);
>    }
> 
> +  /*
> +    Detach temporary MERGE children from temporary parent to allow new
> +    attach at next open. Do not do the detach, if close_thread_tables()
> +    is called from a sub-statement. The temporary table might still be
> +    used in the top-level statement.
> +  */
> +  for (table= thd->temporary_tables; table; table= table->next)
> +  {
> +    if ((table->query_id == thd->query_id) &&
> +        (table->child_l || table->parent))
> +      detach_merge_children(table, TRUE);
> +  }

Davi has introduced a new function into the server,
mark_temp_tables_as_free_for_reuse. Please move this hunk there, if
possible (after the merge with the latest tree).

> +    /* Assert that MERGE children are not attached in unused_tables. */
> +    DBUG_ASSERT((!table->child_l && !table->parent) ||
> +                (table->child_l && !table->children_attached) ||
> +                (table->parent &&
> !table->parent->children_attached));
> +

Perhaps this place and others are similar to
DBUG_ASSERT(! table->has_attached_children);

> @@ -1835,31 +1972,73 @@ void unlink_open_table(THD *thd, TABLE *
>  {
>    char key[MAX_DBKEY_LENGTH];
>    uint key_length= find->s->table_cache_key.length;
> -  TABLE *list, **prev, *next;
> +  TABLE *list, **prev;
>    DBUG_ENTER("unlink_open_table");
>  
>    safe_mutex_assert_owner(&LOCK_open);
>  
> -  list= thd->open_tables;
> -  prev= &thd->open_tables;
>    memcpy(key, find->s->table_cache_key.str, key_length);
> -  for (; list ; list=next)
> +  /*
> +    Note that we need to hold LOCK_open while changing the
> +    open_tables list. Another thread may work on it.
> +    (See: remove_table_from_cache(), mysql_wait_completed_table())
> +    Closing a MERGE child before the parent would be fatal if the
> +    other thread tries to abort the MERGE lock in between.
> +  */
> +  for (prev= &thd->open_tables; *prev; )
>    {
> -    next=list->next;
> +    list= *prev;
> +
>      if (list->s->table_cache_key.length == key_length &&
>  	!memcmp(list->s->table_cache_key.str, key, key_length))
>      {
>        if (unlock && thd->locked_tables)
> -        mysql_lock_remove(thd, thd->locked_tables, list, TRUE);
> +        mysql_lock_remove(thd, thd->locked_tables,
> +                          list->parent ? list->parent : list, TRUE);

Please consider moving the below if/else into a function:

> +      if (list->parent)
> +      {
> +        /* If MERGE child, close parent too. Closing includes detaching. */
> +        TABLE *parent= list->parent;
> +        TABLE **prv_p;
> +
> +        /* Find parent in open_tables list. */
> +        for (prv_p= &thd->open_tables;
> +             *prv_p && (*prv_p != parent);
> +             prv_p= &(*prv_p)->next) {}
> +        if (*prv_p)
> +        {
> +          /* Special treatment required if child follows parent in list. */
> +          if (prev == &parent->next)
> +            prev= prv_p;
> +          /* Remove parent from open_tables list and close it. */
> +          close_thread_table(thd, prv_p);
> +        }
> +      }
> +      else if (list->child_l)
> +      {
> +        /*
> +          When closing a MERGE parent, detach the children first. It is
> +          not necessary to clear the child or parent table reference of
> +          this table because the TABLE is freed. But we need to clear
> +          the child or parent references of the other belonging tables
> +          so that they cannot be moved into the unused_tables chain with
> +          these pointers set.
> +        */
> +        detach_merge_children(list, TRUE);
> +      }

Please provide a comment why this needs to be done and
an SQL example (in the comment, if it's not too big) when this
code is in use.

> +  /*
> +    When all tables are open again, we can re-attach MERGE children to
> +    their parents. All TABLE objects are still present.
> +  */
> +  DBUG_PRINT("tcache", ("re-attaching MERGE tables: %d", merge_table_found));
> +  if (!error && merge_table_found)
> +  {
> +    for (table= thd->open_tables; table; table= next)
> +    {
> +      next= table->next;
> +      DBUG_PRINT("tcache", ("check table: '%s'.'%s' 0x%lx  next: 0x%lx",
> +                            table->s->db.str, table->s->table_name.str,
> +                            (long) table, (long) next));
> +      /* Reattach children for MERGE tables with "closed data files" only. */
> +      if (table->child_l && !table->children_attached)
> +      {
> +        DBUG_PRINT("tcache", ("MERGE parent, attach children"));
> +        if(table->file->extra(HA_EXTRA_ATTACH_CHILDREN))
> +        {
> +          my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias);
> +          VOID(hash_delete(&open_cache, (uchar*) table));
> +          error=1;
> +        }
> +        else
> +        {
> +          table->children_attached= TRUE;
> +          DBUG_PRINT("myrg", ("attached parent: '%s'.'%s' 0x%lx",
> +                              table->s->db.str,
> +                              table->s->table_name.str, (long) table));
> +        }
> +      }
> +    }
> +  }

Please move this into a function, if possible (this is the end of
reopen_tables()).
Perhaps deletion from the open_cache (this line:
+          VOID(hash_delete(&open_cache, (uchar*) table));)
should be outside this new function.


> +/**
> +  @brief Attach MERGE children to the parent.
> +
> +  @detail When the last MERGE child has just been opened, let the
> +    handler attach the MyISAM tables to the MERGE table. Remove MERGE
> +    TABLE_LIST chain from the statement list so that it cannot be
> +    changed or freed.
> +
> +  @param[in]    tlist           the TABLE_LIST object just opened
> +
> +  @return status
> +    @retval     0               OK
> +    @retval     != 0            Error
> +*/
> +
> +static int attach_merge_children(TABLE_LIST *tlist)
> +{
> +  TABLE *parent= tlist->parent_l->table;
> +  int error;
> +  DBUG_ENTER("attach_merge_children");
> +  DBUG_PRINT("myrg", ("table: '%s'.'%s' 0x%lx", parent->s->db.str,
> +                      parent->s->table_name.str, (long) parent));
> +
> +  /* Must not call this with attached children. */
> +  DBUG_ASSERT(!parent->children_attached);
> +  /* Must call this with children list in place. */
> +  DBUG_ASSERT(tlist->parent_l->next_global == parent->child_l);
> +
> +  /* Attach MyISAM tables to MERGE table. */
> +  error= parent->file->extra(HA_EXTRA_ATTACH_CHILDREN);
> +
> +  /*
> +    Remove children from the table list. Even in case of an error.
> +    This should prevent tampering with them.
> +  */
> +  tlist->parent_l->next_global= *parent->child_last_l;
> +
> +  /*
> +    Do not fix the last childs next_global pointer. It is needed for
> +    stepping to the next table in the enclosing loop in open_tables().
> +    Do not fix prev_global pointers. We did not set them.
> +  */
> +
> +  if (error)
> +  {
> +    DBUG_PRINT("error", ("attaching MERGE children failed: %d", my_errno));
> +    parent->file->print_error(error, MYF(0));
> +    DBUG_RETURN(1);
> +  }
> +
> +  parent->children_attached= TRUE;
> +  DBUG_PRINT("myrg", ("attached parent: '%s'.'%s' 0x%lx", parent->s->db.str,
> +                      parent->s->table_name.str, (long) parent));
> +
> +  /*
> +    Note that we have the cildren in the thd->open_tables list at this
> +    point.
> +  */
> +
> +  DBUG_RETURN(0);
> +}

Please add a test case with a trigger on the child, that uses a
table.

Please test under LOCK TABLES.

In other words:

create table t1 () engine= myisam;
create table t2 () engine= myisam;
create table t3 () engine= myisam;
create trigger after insert on t2 ... insert into t3;
create table myrg () union (t1, t2);
lock table myrg write;
insert into t3 ... // must fail
insert into myrg // must not fire the trigger
unlock tables;

Thanks.

Please also add a test case for recursive definitions of merge
tables, one of those you tried when we tried on IRC and got a CPU
hog.

> +bool fix_merge_after_open(TABLE_LIST *old_child_list, TABLE_LIST **old_last,
> +                          TABLE_LIST *new_child_list, TABLE_LIST **new_last)
> +{
> +  bool mismatch= FALSE;
> +  DBUG_ENTER("fix_merge_after_open");
> +  DBUG_PRINT("myrg", ("old last addr: 0x%lx  new last addr: 0x%lx",
> +                      (long) old_last, (long) new_last));
> +
> +  for (;;)
> +  {
> +    DBUG_PRINT("myrg", ("old list item: 0x%lx  new list item: 0x%lx",
> +                        (long) old_child_list, (long) new_child_list));
> +    /* Break if one of the list is at its end. */
> +    if (!old_child_list || !new_child_list)
> +      break;
> +    /* Old table has references to child TABLEs. */
> +    DBUG_ASSERT(old_child_list->table);
> +    /* New table does not yet have references to child TABLEs. */
> +    DBUG_ASSERT(!new_child_list->table);
> +    DBUG_PRINT("myrg", ("old table: '%s'.'%s'  new table: '%s'.'%s'",
> +                        old_child_list->db, old_child_list->table_name,
> +                        new_child_list->db, new_child_list->table_name));
> +    /* Child db.table names must match. */
> +    if (strcmp(old_child_list->table_name, new_child_list->table_name) ||
> +        strcmp(old_child_list->db,         new_child_list->db))
> +    {
> +      mismatch= TRUE;
> +      break;
> +    }
> +    /*
> +      Copy TABLE reference. Child TABLE objects are still in place
> +      though not necessarily open yet.
> +    */
> +    DBUG_PRINT("myrg", ("old table ref: 0x%lx  replaces new table ref: 0x%lx",
> +                        (long) old_child_list->table,
> +                        (long) new_child_list->table));
> +    new_child_list->table= old_child_list->table;
> +    /*
> +      Break if one of the lists is at its last child. When the child
> +      list is part of the statement list, the list may continue after
> +      the last child. The test for NULL is insufficient.
> +    */
> +    DBUG_PRINT("myrg", ("old next addr: 0x%lx  new next addr: 0x%lx",
> +                        (long) &old_child_list->next_global,
> +                        (long) &new_child_list->next_global));
> +    if ((old_last && (&old_child_list->next_global == old_last)) ||
> +        (new_last && (&new_child_list->next_global == new_last)))
> +      break;
> +    /* Step both lists. */
> +    old_child_list= old_child_list->next_global;
> +    new_child_list= new_child_list->next_global;
> +  }
> +  DBUG_PRINT("myrg", ("end of list, mismatch: %d", mismatch));
> +  if (!mismatch &&
> +      (/* old list at end */
> +       (!old_child_list || (!old_last && !old_child_list->next_global)
> ||
> +        (old_last && (&old_child_list->next_global == old_last)))
> &&
> +       /* and new list not at end */
> +       (new_child_list && new_child_list->next_global &&
> +        (!new_last || (&new_child_list->next_global != new_last)))) ||
> +      (/* new list at end */
> +       (!new_child_list || (!new_last && !new_child_list->next_global)
> ||
> +        (new_last && (&new_child_list->next_global == new_last)))
> &&
> +       /* and old list not at end */
> +       (old_child_list && old_child_list->next_global &&
> +        (!old_last || (&old_child_list->next_global != old_last)))))
> +    mismatch= TRUE;
> +  if (mismatch)
> +    my_error(ER_TABLE_DEF_CHANGED, MYF(0));


Could we perhaps simplify this check (discussed on IRC some ideas for it).

> +TABLE *open_n_lock_single_table(THD *thd, TABLE_LIST *table_l,
> +                                thr_lock_type lock_type)

OK, the need for this function is understood.

I, however, think that it should not be used in some places where it is used:
 * in the derived thread: we're going to get ER_WRONG_OBJECT from
   open_ltable anyway. I think it's OK if the error code for this
   situation changes.

 * create_table_from_dump, mysql_table_dump(): I can't see how it
   could have worked before.  COM_TABLE_DUMP is trying to create the
   table with the same definition on slave, and then insert data
   into it. But for a merge table, the slave might not have the 
   right parents, so it becomes order dependent.
   Since this code is scheduled for removal in 6.0, should be OK to 
   do anything with it. Thus your change is OK too.

> -      for (table= tables; table != first_not_own; table= table->next_global)
> +      /*
> +        When open_and_lock_tables() is called for a single table out of
> +        a table list, the 'next_global' chain is temporarily broken. We
> +        may not find 'first_not_own' before the end of the "list".
> +        Look for example at those places where open_n_lock_single_table()
> +        is called. That function implements the temporary breaking of
> +        a table list for opening a single table.
> +      */
> +      for (table= tables;
> +           table && table != first_not_own;
> +           table= table->next_global)

It is difficult to deduce that this comment relates to the
additional check for 'table' in the 'for' loop.

Frankly speaking, temporarily cutting off the list of tables is a
hack, and additional comments won't clarify this hack.

Hacks, instead, should not be used. 

For mysql_alter_table we're guaranteed to have only one table in
the list.

CHECKSUM table can use the same strategy as mysql_admin_table --
terminate the table list manually. Or, better, we could create a
stack copy of the TABLE_LIST to contain only the table in
question.

Right now mysql_admin_table terminates the table list -- is not
this a source of the same bug for which you have added an
additional check and a comment here?

For others, using open_ltable should be fine in my view.

> @@ -3381,9 +3383,50 @@ static TABLE *create_table_from_items(TH
>          else
>            table= create_table->table;
>          VOID(pthread_mutex_unlock(&LOCK_open));
> +
> +        /*
> +          Non-temporary MERGE parent and children need be open, attached
> +          and locked for select. The children have been opened and
> +          locked at statement begin through the UNION=() clause of the
> +          CREATE TABLE part of the statement. A simple CREATE TABLE does
> +          not open any table. But CREATE...SELECT opens all tables,
> +          including the MERGE child tables.
> +
> +          We cannot use the existing locks of the children directly. For
> +          replication we need an 'extra_lock' on the parent table. We
> +          cannot first unlock the children and then lock them through
> +          the parent. This would allow other threads to step in.
> +
> +          So we need new TABLE objects for the children so that they can
> +          be locked through the parent. Later we remove the locks from
> +          the original child TABLEs.
> +        */
> +        if (table && table->child_l)
> +        { > +          uint counter;
> +          /* Assert that child list is terminated. */
> +          DBUG_ASSERT(!*table->child_last_l);
> +          if (open_tables(thd, &create_table->table->child_l,
> &counter,
> +                          MYSQL_LOCK_IGNORE_FLUSH) ||
> +              table->file->extra(HA_EXTRA_ATTACH_CHILDREN))
> +          {
> +            if (!create_info->table_existed)
> +              drop_open_table(thd, table, create_table->db,
> +                              create_table->table_name);
> +            table= NULL;
> +          }
> +          else
> +          {
> +            table->children_attached= TRUE;
> +            DBUG_PRINT("myrg", ("attached parent: '%s'.'%s' 0x%lx",
> +                                table->s->db.str,
> +                                table->s->table_name.str, (long) table));
> +          }
> +        }

Yuck. Let's prohibit CREATE TABLE .. SELECT for merge tables instead.
I checked this with engineers on #support.

It doesn't work now anyway:

mysql> create table t1 (a int, b int);
Query OK, 0 rows affected (0.02 sec)

mysql> create table t2 (a int, b int);
Query OK, 0 rows affected (0.01 sec)

mysql> create table mrg (a int, b int) union (t1, t2) engine=merge select 1 as a, 2 as
> b;
ERROR 1213 (40001): Deadlock found when trying to get lock; try restarting transaction

Note that slightly different syntax works and produces wrong results:

mysql> create table mrg (a int, b int) union (t1, t2)  select 1 as a, 2 as b;
Query OK, 1 row affected (0.02 sec)
Records: 1  Duplicates: 0  Warnings: 0

mysql> show create table mrg\G
*************************** 1. row ***************************
       Table: mrg
Create Table: CREATE TABLE `mrg` (
  `a` int(11) DEFAULT NULL,
  `b` int(11) DEFAULT NULL
) ENGINE=MyISAM DEFAULT CHARSET=latin1
1 row in set (0.00 sec)

So, please prohibit this and add a test case showing that it is
unsupported.
This should be documented in the manual, too.


> +static bool check_merge_table_access_fix_locks(THD *thd, char *db,
> +                                               TABLE_LIST *table_list)
> +{
> +  int error= 0;
> +
> +  if (table_list)
> +  {
> +    /* Check that all tables use the current database */
> +    TABLE_LIST *tlist;
> +
> +    for (tlist= table_list; tlist; tlist= tlist->next_local)
> +    {
> +      /*
> +        We create or alter a MERGE table. In both cases the MERGE child
> +        tables must be locked in write mode.
> +      */
> +      tlist->lock_type= TL_WRITE;
> +
> +      if (!tlist->db || !tlist->db[0])
> +        tlist->db= db; /* purecov: inspected */
> +    }
> +    error= check_table_access(thd, SELECT_ACL | UPDATE_ACL | DELETE_ACL,
> +                              table_list,0);
> +  }
> +  return error;
> +}

The old code was fine I believe. If the sets the wrong lock, the
parser should be fixed instead.

This is where it could be done:

      | UNION_SYM opt_equal '(' table_list ')'
          {
            /* Move the union list to the merge_list */
            LEX *lex=Lex;
            TABLE_LIST *table_list= lex->select_lex.get_table_list();
            lex->create_info.merge_list= lex->select_lex.table_list;
            lex->create_info.merge_list.elements--;
            lex->create_info.merge_list.first=
              (uchar*) (table_list->next_local);


In any case we should not mix privilege checking and lock setting
in the same function - these functions belong to different
subsystems.

> @@ -3820,6 +3827,16 @@ static int prepare_for_restore(THD* thd,
>      DBUG_RETURN(send_check_errmsg(thd, table, "restore",
>                                    "Failed to open partially restored table"));
>    }
> +  if (table->table && table->table->child_l)
> +  {
> +    /* A MERGE table must not come here. */
> +    /* purecov: begin inspected */
> +    unlock_table_name(thd, table);
> +    pthread_mutex_unlock(&LOCK_open);
> +    DBUG_RETURN(send_check_errmsg(thd, table, "restore",
> +                                  "Not supported for MERGE tables"));
> +    /* purecov: end */
> +  }
>    pthread_mutex_unlock(&LOCK_open);
>    DBUG_RETURN(0);
>  }
> @@ -3862,6 +3879,17 @@ static int prepare_for_repair(THD *thd, 
>      table= &tmp_table;
>      pthread_mutex_unlock(&LOCK_open);
>    }
> +
> +  if (table->child_l)
> +  {
> +    /* A MERGE table must not come here. */
> +    /* purecov: begin inspected */
> +    error= send_check_errmsg(thd, table_list, "repair",
> +                             "Not supported for MERGE tables");
> +    goto end;
> +    /* purecov: end */
> +  }

I understand and support this intention. But bailing out in the
prepare function is a bit too late in my opinion.
Can we instead of having a check here pass the
flag to the storage engine at open time, in extra_open_options
(thd->open_options)? If the flag is present, open should fail.
This should ensure that we never issue a lock upgrade for merge
tables.

 
> +
> +MI_INFO *myisam_engine_handle(handler *handler_handle)
> +{
> +  DBUG_ENTER("myisam_engine_handle");
> +  if (handler_handle->ht->db_type != DB_TYPE_MYISAM)
> +  {
> +    DBUG_PRINT("error", ("not a MyISAM table"));
> +    DBUG_RETURN(NULL);
> +  }
> +  DBUG_RETURN(((ha_myisam*) handler_handle)->file);
> +}

I believe you should either declare a getter in ha_myisam for
MI_INFO and remove the friend, or move declaration and
implementation of the friend to myisammrg.

I think a friend that is publicly accessible in sql/ provides
no additional encapsulation and only adds maintenance burden.

Dmitri says he asked you to do that. I am sure this bikeshed issue
won't prevent us from pushing the  patch.

--------------------------------------------------------------
This is all. I have no comments for the myisammrg part, it is
cleanly designed and well implemented, and I have not been
looking for coding bugs.

Thank you again, and let's discuss the review on IRC.

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY