On 06/07/11 21:46, Davi Arnaut wrote:
> On 6/7/11 10:29 AM, Alexander Nozdrin wrote:
>> #At file:///home/alik/MySQL/bzr/00/bug55843/mysql-trunk-bug55843-01/ based on
> revid:alexander.nozdrin@stripped
>>
>> 3167 Alexander Nozdrin 2011-06-07
>> Pre-requisite patch for Bug#11763162 (55843 - Handled condition
>> appears as not handled).
>>
>> The patch changes the relationship between THD, Diagnostics_area
>> and Warning_info classes:
>>
>> - before the patch, THD owned both Diagnostics_area and
>> Warning_info instances.
>>
>> - after the patch THD owns Diagnostics_area instance,
>> and Diagnostics_area owns Warning_info instance.
>
> What will be the consequence of this to code that manipulated only of of
> the two?
It's been discussed, and I thought, we agreed that Warning_info is a kind
of implementation details of Diagnostics_area. They are quite interdependent
anyway.
I'm doing this because in later patches, a pointer from Diagnostics_area
to an object in its Warning_info instance will be introduced.
> Did we have any case where a Diagnostics_area was pushed but not
> a new Warning_info area?
I've found two cases when Diagnostics_are is pushed:
- sql_prepare.cc
Here it's pushed along with a new instance of Warning_info.
- rpl_master.cc
Here Diagnostics_area is pushed alone.
The comment says:
Dump thread sends ER_MASTER_FATAL_ERROR_READING_BINLOG instead of the real
errors happend on master to slave when erorr is encountered.
So set a temporary Diagnostics_area to thd. The low level error is always
set into the temporary Diagnostics_area and be ingored. The original
Diagnostics_area will be restored at the end of this function.
ER_MASTER_FATAL_ERROR_READING_BINLOG will be set to the original
Diagnostics_area.
Not sure if that's a proper usage...
> or vice versa?
There are cases in sp_head.cc, sql_admin.cc, sql_show.cc.
The thing there is to provide a separate Warning_info for some "execution"
and ignore/process thrown warnings later.
> or only the main warning info
> area is now owned by Diagnostics_area?
Err... I'm not sure I get the question...
>> The patch changes THD::get_warning_info() so that it still
>> works (to save code changes) and eliminates THD::set_warning_info().
>> Users should use Diagnostics_area::set_warning_info() instead.
>
> Please also explain why this is necessary and what is the ultimate goal
> to be achieved. Otherwise, it's a quite hard to see where we are going
> with those changes.
Hmm.. On the second thought, probably, I should leave THD::set_warning_info()
in this patch and remove it later (along with the THD::get_warning_info()).
Will something like this be Ok:
The goal of this patch is to change the relationship between THD,
Diagnostics_area and Warning_info classes:
- before the patch, THD owned both Diagnostics_area and
Warning_info instances.
- after the patch THD owns Diagnostics_area instance,
and Diagnostics_area owns Warning_info instance.
The patch changes THD::get_warning_info() and THD::set_warning_info()
so that they still work to minimize side changes.
Those functions will be removed later in a separate patch.
>> === modified file 'sql/rpl_master.cc'
>> --- a/sql/rpl_master.cc 2011-06-07 13:09:47 +0000
>> +++ b/sql/rpl_master.cc 2011-06-07 13:29:37 +0000
>> @@ -648,7 +648,7 @@ void mysql_binlog_send(THD* thd, char* l
>> ER_MASTER_FATAL_ERROR_READING_BINLOG will be set to the original
>> Diagnostics_area.
>> */
>> - Diagnostics_area temp_da;
>> + Diagnostics_area temp_da(0, false);
>
> Preserve the original constructor.
Ok.
