From: Alexander Nozdrin
Date: January 19 2011 12:47pm
Subject: Re: bzr commit into mysql-trunk-bugfixing branch (alexander.nozdrin:3421)
 Bug#55847
List-Archive: http://lists.mysql.com/commits/129184
Message-Id: <4D36DD65.3060308@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Hi Davi,

thanks for the review. I'll submit a new patch shortly.
See also comments below.

On 01/17/11 12:18, Davi Arnaut wrote:
>>         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.
> 
> What was the consequence? I mean, I agree this is hack, nonetheless, it
> would be nice to have an explanation of the specific problem.

Ultimately, that is needed for a fix for Bug#55843.
I'll add more notes to new changeset comment.

>> === modified file 'sql/sql_admin.cc'
>> --- a/sql/sql_admin.cc	2010-11-18 16:34:56 +0000
>> +++ b/sql/sql_admin.cc	2010-12-11 20:17:36 +0000
>> @@ -336,13 +336,36 @@ static bool mysql_admin_table(THD* thd,
>>            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;
>> +      Diagnostics_area da;
>> +      Warning_info wi(thd->query_id);
>> +      Diagnostics_area *da_saved= thd->stmt_da;
>> +      Warning_info *wi_saved= thd->warning_info;
> 
> Don't we just need to save and restore warning_info?

You're right. Removing here, there and elsewhere.

>> === modified file 'sql/sql_parse.cc'
>> --- a/sql/sql_parse.cc	2010-11-29 11:28:55 +0000
>> +++ b/sql/sql_parse.cc	2010-12-11 20:17:36 +0000
>> @@ -7267,6 +7267,7 @@ bool parse_sql(THD *thd,
>>      /* Check that if MYSQLparse() failed, thd->is_error() is set. */
>>
>>      DBUG_ASSERT(!mysql_parse_status ||
>> +              (mysql_parse_status&&   thd->get_internal_handler()) ||
> 
> Why was the assert changed? Please write per-file comments.

It's coupled with Trigger_error_handler in sql_show.cc. See below.

>> +              Trigger_error_handler err_handler;
>> +              thd->push_internal_handler(&err_handler);
>> +
>> +              int res= fill_schema_table_from_frm(thd, tables, schema_table, db_name,
>> +                                                  table_name, schema_table_idx,
>> +                                                  can_deadlock);
> 
> I don't understand this change.
> 
> Now it's safe to generate warning messages, except for those that are
> handled? What happens if one of these errors is raised but are ignored?
> It seems they weren't being exactly ignored before.

It is/was a bit messy.

The thing is that when triggers are loaded, several errors/warnings
might happen. In particular, the following ones had special "handling":

  - ER_PARSE_ERROR -- obviously happens when trigger definition
    is garbled. The logic was as follows:
      - set the magic flag (no_warnings_for_error)
      - parsing error
      - throw an error
      - set error state in THD
      - return error from MYSQLparse()
      - check that there is an error, and if that's an ER_PARSE_ERROR, clear it
      - return (trigger is not loaded, no error; thus invalid trigger is
        just skipped)
      - reset the magic flag

  - ER_TRG_NO_DEFINER & ER_TRG_NO_CREATION_CTX -- check_n_load() used to
    throw those warnings unless no_warnings_for_error is set.

So, all in all, this internal error handler is to silence these three
SQL-conditions, which were silenced using no_warnings_for_error hack.

> 
>> +              thd->pop_internal_handler();
> 
> Please add a test case for this change.

There is a test case for that -- it's in trigger tests.

If you remove any line of that new code, some trigger test will start
to fail immediately.

> You could make this into a visitor pattern...
> 
>> +        thd->warning_info->append_warnings(thd,&wi.warn_list());
> 
> which could be passed through append_warnings.

I'm not sure, I fully catch your idea.If that is what I think, I'd procrastinate
on that until there are more use cases. Could you sketch it put plz?