List:Commits« Previous MessageNext Message »
From:Jon Olav Hauglid Date:March 23 2011 11:02am
Subject:Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3306) Bug#11763166
View as plain text  
Hello,

Patch looks ok, but a few comments below.

On 03/22/2011 03:36 PM, Alexander Nozdrin wrote:
>   3306 Alexander Nozdrin	2011-03-22
>        A patch for Bug#11763166 (55847: SHOW WARNINGS returns empty
>        result set when SQLEXCEPTION is active.
>
>        The problem was in a hackish THD::no_warnings_for_error attribute.
>        When it was set, an error was not written to Warning_info -- only
>        Diagnostics_area state was changed. That means, Diagnostics_area
>        might contain error state, which is not present in Warning_info.
>
>        The user-visible problem was that in some cases SHOW WARNINGS
>        returned empty result set (i.e. there were no warnings) while
>        the previous SQL statement failed. According to the MySQL
>        protocol errors must be presented in warning list.
>
>        The fix is to remove THD::no_warnings_for_error.

Generally I like your changeset comment and code comments.

But I think this last sentence should be extended. The fix isn't just
to remove THD::no_warnings_for_error, otherwise the patch would
have been much simpler :-)

>        This patch is needed to fix Bug 55843.

Maybe mention the Oracle bug no as well? (11763162)

> === modified file 'sql/sql_admin.cc'
> --- a/sql/sql_admin.cc	2011-03-08 09:21:39 +0000
> +++ b/sql/sql_admin.cc	2011-03-22 14:36:29 +0000
> @@ -331,18 +331,43 @@ static bool mysql_admin_table(THD* thd,
>         lex->query_tables= table;
>         lex->query_tables_last=&table->next_global;
>         lex->query_tables_own_last= 0;
> -      /*
> -        Under locked tables, we know that the table can be opened,
> -        so any errors opening the table are logical errors.
> -        In these cases it makes sense to report them.
> -      */
> -      if (!thd->locked_tables_mode)
> -        thd->no_warnings_for_error= no_warnings_for_error;
> +
>         if (view_operator_func == NULL)
>           table->required_type=FRMTYPE_TABLE;
>
> -      open_error= open_and_lock_tables(thd, table, TRUE, 0);
> -      thd->no_warnings_for_error= 0;
> +      if (!thd->locked_tables_mode&&  repair_table_use_frm)
> +      {
> +        /*
> +          If we're not under LOCK TABLES and we're executing REPAIR TABLE
> +          USE_FRM, we need to ignore errors from open_and_lock_tables().
> +          REPAIR TABLE USE_FRM is a heavy weapon used when a table is
> +          critically damaged, so open_and_lock_tables() will most likely
> +          report errors. Those errors are not interesting for the user
> +          because it's already known that the table is badly damaged.
> +        */
> +
> +        Warning_info wi(thd->query_id);
> +        Warning_info *wi_saved= thd->warning_info;
> +
> +        thd->warning_info=&wi;
> +
> +        open_error= open_and_lock_tables(thd, table, TRUE, 0);
> +
> +        thd->warning_info= wi_saved;
> +      }
> +      else
> +      {
> +        /*
> +          It's assumed that even if it is REPAIR TABLE USE_FRM, the table
> +          can be opened if we're under LOCK TABLES (otherwise LOCK TABLES
> +          would fail). Thus, the only errors we could have from
> +          open_and_lock_tables() are about improper locking mode and stuff.

Please adjust this comment like discussed on IRC.

> +          It does make sense for the user to see such errors.
> +        */
> +
> +        open_error= open_and_lock_tables(thd, table, TRUE, 0);
> +      }
> +
>         table->next_global= save_next_global;
>         table->next_local= save_next_local;
>         thd->open_options&= ~extra_open_options;
>

> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h	2011-03-11 09:35:38 +0000
> +++ b/sql/sql_class.h	2011-03-22 14:36:29 +0000
> @@ -2789,6 +2788,16 @@ public:
>     inline Internal_error_handler *get_internal_handler()
>     { return m_internal_handler; }
>
> +#ifndef DBUG_OFF
> +  /**
> +    Indicates if an internal error handler processed SQL condition.
> +    This operation is intended for debugging only and is used in
> +    parse_sql() in DBUG_ASSERT() macro.
> +  */
> +  inline bool is_condition_handled() const
> +  { return m_condition_handled; }
> +#endif
> +

I know I sort of requested something like this ...

But I think this is ends up being too much just to make an assert
more accurate.There's also a potential problem if more than one
error occurs and only one of them is handled by the error 
handler.(m_condition_handled will only show if the last error was handled
or not)

These changes also make release and debug builds more different and they 
are different enough as is, imo.

So I suggest that this change (and the related code) is reverted and
the assert comment adjusted instead to say that it will not catch a
situation where parsing fails without an error reported if an
error handler exists.

What do you think?

> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc	2011-03-11 18:53:12 +0000
> +++ b/sql/sql_show.cc	2011-03-22 14:36:29 +0000
> @@ -3414,6 +3414,45 @@ end:
>
>
>   /**
> +  Trigger_error_handler is intended to intercept and silence SQL conditions
> +  that might happen during trigger loading for SHOW statements.
> +  The potential SQL conditions are:
> +
> +    - ER_PARSE_ERROR -- this error is thrown if a trigger definition file
> +      is damaged or contains invalid CREATE TRIGGER statement. That should
> +      not happen in normal life.
> +
> +    - ER_TRG_NO_DEFINER -- this warning is thrown when we're loading a
> +      trigger created/imported in/from the version of MySQL, which does not
> +      support trigger definers.
> +
> +    - ER_TRG_NO_CREATION_CTX -- this warning is thrown when we're loading a
> +      trigger created/imported in/from the version of MySQL, which does not
> +      support trigger creation contexts.
> +*/

Thanks for adding this comment :-)

> +/**
> +  Fill INFORMATION_SCHEMA-table, leave correct Diagnostics_area /
> +  Warning_info state after itself.
> +
> +  This function is a wrapper around ST_SCHEMA_TABLE::fill_table(), which
> +  may "partially silence" some errors. The thing is that during
> +  fill_table() many errors might be emitted. These errors stem from the
> +  nature of fill_table().
> +
> +  For example, SELECT ... FROM INFORMATION_SCHEMA.xxx WHERE TABLE_NAME = 'xxx'
> +  results in a number of 'Table<db name>.xxx does not exist' errors,
> +  because fill_table() tries to open the 'xxx' table in every possible
> +  database.
> +
> +  Those errors are cleared (the error status is cleared from
> +  Diagnostics_area) inside fill_table(), but they remain in Warning_info
> +  (Warning_info is not cleared because it may contain useful warnings).

Maybe you could say that this function is responsible for making sure 
that Warning_info does not contain warnings corresponding to the cleared 
errors?

> +  THD::no_warnings_for_error used to be set before calling fill_table(),
> +  thus those errors didn't go to Warning_info. This is not the case now
> +  (THD::no_warnings_for_error was eliminated as a hack), so we need to take
> +  care of those warnings here.

I think this last paragraph can be removed. Explaining how it used to 
work won't make much sense to people reading this code after your patch 
has been pushed.

Otherwise the patch looks good and I think it can be pushed pending any 
review comments from Davi.

Thanks,

--- Jon Olav
Thread
bzr commit into mysql-trunk branch (alexander.nozdrin:3306) Bug#11763166Alexander Nozdrin22 Mar
  • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3306) Bug#11763166Jon Olav Hauglid23 Mar
  • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3306) Bug#11763166Davi Arnaut25 Mar
    • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3306) Bug#11763166Alexander Nozdrin30 Mar