List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:June 22 2011 9:29pm
Subject:Re: bzr commit into mysql-5.5 branch (Dmitry.Lenev:3452) Bug#11754210
View as plain text  
Hi Dmitry,

On 6/21/11 9:15 AM, Dmitry Lenev wrote:
> #At file:///home/dlenev/src/bzr/mysql-5.5-11754210/ based on
> revid:marko.makela@stripped
> 
>  3452 Dmitry Lenev	2011-06-21
>       Tentative fix for bug #11754210 - "45777: CHECK TABLE DOESN'T
>       SHOW ALL PROBLEMS FOR MERGE TABLE COMPLIANCE IN 5.1".
>       
>       The problem was that CHECK/REPAIR TABLE for a MERGE table which
>       had several children missing or in wrong engine reported only
>       issue with the first such table in its result-set. While in 5.0
>       this statement returned the whole list of problematic tables
>       in this case.

s/in this case//.

>       Ability to report problems for all children was lost during
>       significant refactorings of MERGE code which were done as part
>       of work on 5.1 and 5.5 releases.
>       
>       This patch restores status quo ante refactorings by changing
>       code in such a way that:
>       1) Failure to open child table during CHECK/REPAIR TABLE for
>          a MERGE table is not reported immediately when its absence
>          is discovered in open_tables(). Instead handling/error
>          reporting in such a situation is postponed until the moment
>          when children are attached.
>       2) Code performing attaching of children no longer stops when
>          it encounters first problem with one of the children during
>          CHECK/REPAIR TABLE. Instead it continues iteration through
>          the child list until all problems caused by child absence/
>          wrong engine are reported.
>       
>       Note that even after this change problem with mismatch of
>       child/parent definition won't be reported if there is also
>       another child missing, but this is how it was in 5.0 as well.
>      @ mysql-test/r/merge.result
>         Added test case for bug #11754210 - "45777: CHECK TABLE DOESN'T
>         SHOW ALL PROBLEMS FOR MERGE TABLE COMPLIANCE IN 5.1".
>         Adjusted results of existing tests to the fact that CHECK/REPAIR
>         TABLE statements now try to report problems about missing table/
>         wrong engine for all underlying tables, and to the fact that
>         mismatch of parent/child definitions is always reported as an
>         error and not a warning.

>      @ mysql-test/t/merge.test
>         Added test case for bug #11754210 - "45777: CHECK TABLE DOESN'T
>         SHOW ALL PROBLEMS FOR MERGE TABLE COMPLIANCE IN 5.1".
>      @ sql/sql_base.cc
>         Changed code responsible for opening tables to ignore the fact
>         that underlying tables of a MERGE table are missing, if this
>         table is opened for CHECK/REPAIR TABLE.
>         The absence of underlying tables in this case is now detected and
>         appropriate error is reported at the point when child tables are
>         attached. At this point we can produce full list of problematic
>         child tables/errors to be returned as part of CHECK/REPAIR TABLE
>         result-set.
>      @ storage/myisammrg/ha_myisammrg.cc
>         Changed myisammrg_attach_children_callback() to handle new
>         situation, when during CHECK/REPAIR TABLE we do not report 
>         error about missing child immediately when this fact is 
>         discovered during open_tables() but postpone error-reporting
>         till the time when children are attached. 
>         Also this callback is now responsible for pushing an error
>         mentioning problematic child table to the list of errors to 
>         be reported by CHECK/REPAIR TABLE statements.
>         
>         Changed myrg_print_wrong_table() to always report a problem
>         with child table as an error and not as a warning.

One "problem" is that storage engines have a specific interface for
reporting errors, which is handler::print_error(). Engines shouldn't
invoke my_error() outside of print_error.

See Bug#53696 for an example.

It would be more kosher if the error could be reported through a
ha_myisammrg specific print_error. Please look into this, but it's a not
a strict requirement.

>      @ storage/myisammrg/myrg_open.c
>         Changed code in myrg_attach_children() not to abort on the
>         first problem with a child table when attaching children to
>         parent MERGE table during CHECK/REPAIR TABLE statement 
>         execution. This allows CHECK/REPAIR TABLE to report problems 
>         about absence/wrong engine for all underlying tables as
>         part of their result-set.
> 

[...]

> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2011-05-21 08:21:08 +0000
> +++ b/sql/sql_base.cc	2011-06-21 12:15:22 +0000
> @@ -68,7 +68,7 @@ No_such_table_error_handler::handle_cond
>                                                MYSQL_ERROR ** cond_hdl)
>  {
>    *cond_hdl= NULL;
> -  if (sql_errno == ER_NO_SUCH_TABLE)
> +  if (sql_errno == ER_NO_SUCH_TABLE || sql_errno == ER_WRONG_MRG_TABLE)
>    {
>      m_handled_errors++;
>      return TRUE;
> @@ -4363,13 +4363,25 @@ open_and_process_table(THD *thd, LEX *le
>    /* Not a placeholder: must be a base table or a view. Let us open it. */
>    DBUG_ASSERT(!tables->table);
>  
> -  if (tables->prelocking_placeholder)
> +  if (tables->prelocking_placeholder ||
> +      (tables->parent_l && (thd->open_options &
> HA_OPEN_FOR_REPAIR)))
>    {
>      /*
>        For the tables added by the pre-locking code, attempt to open
>        the table but fail silently if the table does not exist.
>        The real failure will occur when/if a statement attempts to use
>        that table.
> +
> +      Do similar thing for underlying tables of a MERGE table if this
> +      table is opened for CHECK/REPAIR TABLE statement. This is needed
> +      to provide complete list of problematic underlying tables in
> +      CHECK/REPAIR TABLE output.
> +
> +      QQ: Maybe it makes sense to split branch for prelocking case
> +          and MERGE + CHECK TABLES and use different, more specific
> +          error handlers for each case? I.e. currently we somewhat
> +          rely that for tables from prelocking list we won't get
> +          ER_WRONG_MRG_TABLE...

Yes.

>      */
>      No_such_table_error_handler no_such_table_handler;
>      thd->push_internal_handler(&no_such_table_handler);
> 
> === modified file 'storage/myisammrg/ha_myisammrg.cc'
> --- a/storage/myisammrg/ha_myisammrg.cc	2011-04-20 17:53:08 +0000
> +++ b/storage/myisammrg/ha_myisammrg.cc	2011-06-21 12:15:22 +0000
> @@ -159,9 +159,7 @@ extern "C" void myrg_print_wrong_table(c
>    buf[db.length]= '.';
>    memcpy(buf + db.length + 1, name.str, name.length);
>    buf[db.length + name.length + 1]= 0;
> -  push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> -                      ER_ADMIN_WRONG_MRG_TABLE, ER(ER_ADMIN_WRONG_MRG_TABLE),
> -                      buf);
> +  my_error(ER_ADMIN_WRONG_MRG_TABLE, MYF(0), buf);

See remark about handler::print_error.

>  }
>  
>  
> @@ -625,6 +623,23 @@ extern "C" MI_INFO *myisammrg_attach_chi
>    param->next();
>  
>    /*
> +    When MERGE table is opened for CHECK or REPAIR TABLE statements failure

Add a comma after "statements".

> +    to open any of underlying tables is ignored until this moment (this is
> +    needed to provide complete list of problematic underlying tables in
> +    CHECK/REPAIR TABLE output).
> +    Here we detect such a situation and report an appropriate error.
> +  */
> +  if (! child)

Beware when merging to trunk, there is already a similar check.

> +  {
> +    DBUG_PRINT("error", ("failed to open underlying table '%s'.'%s'",
> +                         child_l->db, child_l->table_name));
> +    /* This should only happen inside of CHECK/REPAIR TABLE. */
> +    DBUG_ASSERT(current_thd->open_options & HA_OPEN_FOR_REPAIR);
> +    my_errno= HA_ERR_WRONG_MRG_TABLE_DEF;
> +    goto end;
> +  }
> +
> +  /*
>      Do a quick compatibility check. The table def version is set when
>      the table share is created. The child def version is copied
>      from the table def version after a successful compatibility check.
> @@ -670,6 +685,15 @@ extern "C" MI_INFO *myisammrg_attach_chi
>                        my_errno ? 0L : (long) myisam, my_errno));
>  
>   end:
> +
> +  if (child_l && !myisam &&
> +      (current_thd->open_options & HA_OPEN_FOR_REPAIR))

Perhaps it would be better to just have a auxiliary variable that is set
in the above introduced if condition.

> +  {
> +    char buf[2*NAME_LEN + 1 + 1];
> +    strxnmov(buf, sizeof(buf) - 1, child_l->db, ".", child_l->table_name,
> NULL);
> +    my_error(ER_ADMIN_WRONG_MRG_TABLE, MYF(0), buf);

See remark about print_error.

> +  }
> +
>    DBUG_RETURN(myisam);
>  }
>  
> 
> === modified file 'storage/myisammrg/myrg_open.c'
> --- a/storage/myisammrg/myrg_open.c	2011-02-11 14:00:09 +0000
> +++ b/storage/myisammrg/myrg_open.c	2011-06-21 12:15:22 +0000
> @@ -385,6 +385,7 @@ int myrg_attach_children(MYRG_INFO *m_in
>    uint       UNINIT_VAR(key_parts);
>    uint       min_keys;
>    my_bool    bad_children= FALSE;
> +  my_bool    first_child= TRUE;
>    DBUG_ENTER("myrg_attach_children");
>    DBUG_PRINT("myrg", ("handle_locking: %d", handle_locking));
>  
> @@ -399,16 +400,26 @@ int myrg_attach_children(MYRG_INFO *m_in
>    errpos= 0;
>    file_offset= 0;
>    min_keys= 0;
> -  child_nr= 0;
> -  while ((myisam= (*callback)(callback_param)))
> +  for (child_nr= 0; child_nr < m_info->tables; child_nr++)
>    {
> +    if (! (myisam= (*callback)(callback_param)))
> +    {
> +      if (handle_locking & HA_OPEN_FOR_REPAIR)
> +      {
> +        /* An appropriate error should've been already pushed by callback. */

Assert?

> +        bad_children= TRUE;
> +        continue;
> +      }
> +      goto bad_children;
> +    }
> +
>      DBUG_PRINT("myrg", ("child_nr: %u  table: '%s'",
>                          child_nr, myisam->filename));
> -    DBUG_ASSERT(child_nr < m_info->tables);
>  
>      /* Special handling when the first child is attached. */
> -    if (!child_nr)
> +    if (first_child)
>      {
> +      first_child= FALSE;
>        m_info->reclength= myisam->s->base.reclength;
>        min_keys=  myisam->s->base.keys;
>        key_parts= myisam->s->base.key_parts;
> @@ -456,7 +467,6 @@ int myrg_attach_children(MYRG_INFO *m_in
>      for (idx= 0; idx < key_parts; idx++)
>        m_info->rec_per_key_part[idx]+=
> (myisam->s->state.rec_per_key_part[idx] /
>                                         m_info->tables);
> -    child_nr++;

Hum, in case of "Check table definition match." failure for repair,
child_nr wouldn't be incremented, now it seems to be incremented.

I wonder which one is correct.

Regards,

Davi
Thread
bzr commit into mysql-5.5 branch (Dmitry.Lenev:3452) Bug#11754210Dmitry Lenev21 Jun
  • Re: bzr commit into mysql-5.5 branch (Dmitry.Lenev:3452) Bug#11754210Davi Arnaut23 Jun