From: Alexander Nozdrin
Date: June 8 2011 10:16am
Subject: Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162
List-Archive: http://lists.mysql.com/commits/138828
Message-Id: <4DEF4C13.9030506@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

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.