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?
