List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:June 8 2011 11:09am
Subject:Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162
View as plain text  
On 6/8/11 7:16 AM, Alexander Nozdrin wrote:
> 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.

Yes, we had discussed and agreed, but we also need to make this
information available for whomever did not participate in the
discussion. The form to make this information available to have it
explained in the changeset comment. We need to explain the reason behind
it and what will be the consequences.

> I'm doing this because in later patches, a pointer from Diagnostics_area
> to an object in its Warning_info instance will be introduced.

Yes, please add this to the changeset comment.

>> 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.

Please note these in the per-file changeset comments. We need notes
about what was the previous behavior and what is being done now.

>     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.

Please also note these in appropriate per-file comments.

>> or only the main warning info
>> area is now owned by Diagnostics_area?
> 
> Err... I'm not sure I get the question...

It's a request to clarify if only the main warning info area is owned by
the diagnostics area. If a warning_info can still be pushed, it's not
only owned by the diagnostics area. We might need a better term.

> 
>>>        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.
> 

OK. Please use this in the changeset comment.

Regards,

Davi
Thread
bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162Alexander Nozdrin7 Jun
  • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162Davi Arnaut7 Jun
    • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162Alexander Nozdrin8 Jun
      • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162Davi Arnaut8 Jun
  • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162Jon Olav Hauglid8 Jun
    • Re: bzr commit into mysql-trunk branch (alexander.nozdrin:3167) Bug#11763162Alexander Nozdrin8 Jun