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
