Re: bzr commit into mysql-5.4-bugfixing branch (aelkin:2801) Bug#36524

[Mats Kindahl]

Andrei Elkin wrote:
> Mats, hello.
> 
> The ultimate changeset including all affected result files
> and addressing your critique has been committed:
> 
> http://lists.mysql.com/commits/78315
> 
> 
>> Hi Andrei,
[snip]

>>>    rli->report(level, thd->is_error()? thd->stmt_da->sql_errno()
> : 0,
>> So here you're digging up the error fom the stmt_da...
>>
> 
> Right. This one is the row-basic error summary reporting function.
> 
> [As an offtopic, a future plan is to merge it into the common with other events types
> 
> summary error composition in exec_relay_log_event() 
> 
>    exec_res= apply_event_and_update_pos(ev, thd, rli, TRUE);
>    if (exec_res)
>       rli->report(error in execution of event [providing details here])
> ]

OK.

[snip]

>>>  void
>>>  Slave_reporting_capability::report(loglevel level, int err_code,
>>>                                     const char *msg, ...) const
>>>  {
>>> +  THD *thd= current_thd;
>>>    void (*report_function)(const char *, ...);
>>> -  char buff[MAX_SLAVE_ERRMSG];
>>> +  char buff[sizeof(m_last_error.message)];
>> Very good.
>>
>>>    char *pbuff= buff;
>>> -  uint pbuffsize= sizeof(buff);
>>>    va_list args;
>>> +  
>>> +  if (level == ERROR_LEVEL && has_temporary_error(thd))
> 
>> has_temporary_error() refers to thd->stmt_da->sql_errno(), which may not be
> the
>> same as err_code above. I thought we agreed that you should either pass the
>> error code to has_temporary_error(), or read it from thd->stmt_da to ensure
> that
>> the same error code is used in both cases.
>>
> 
> Yes, that was a slip. Thanks for heads-up!
> 
>> Note that above, at least, you are passing in the error code from stmt_da, so if
>> you do that everywhere, and don't want to support anything else, an easy patch
>> would be to remove the parameter and read the error code from stmt_da and assign
>> it to a local variable in the report() function, something like:
>>
>> {
>>   int err_code = thd->stmt_da->sql_errno();
>>   ...
>>
>> Of course, I am fine with adding the err_code as parameter to
>> has_temporary_error as well.
> 
> ok. I'll go with the err_code arg for now.

OK.

[snip]

>>> +  void display(int err_code, const char *msg, ...) const;
>>> +
>> Please add an ATTRIBUTE_FORMAT(printf, 2, 3) to this...
> 
> thanks 4 explaining why. Should not it be actually 3,4 
> due to the instance of the class 
> is presented in the arg list implicitly?

Right... my mistake.

[snip]

>>> === modified file 'sql/slave.h'
>>> --- a/sql/slave.h	2009-04-30 14:51:06 +0000
>>> +++ b/sql/slave.h	2009-06-29 10:47:20 +0000
>>> @@ -203,6 +203,8 @@ void set_slave_thread_default_charset(TH
>>>  void rotate_relay_log(Master_info* mi);
>>>  int apply_event_and_update_pos(Log_event* ev, THD* thd, Relay_log_info*
> rli,
>>>                                 bool skip);
>>> +int has_temporary_error(THD *thd);
>>> +void clear_all_errors(THD *thd, Relay_log_info *rli);
>> So, you have a mutual dependency between slave.o and rpl_reporting.o. We might
>> have had that before, but maybe you should consider moving has_temporary_error()
>> into rpl_reporting.cc instead, because that is where it is used. However, that
>> is your decision.
>>
> 
> Let's keep the current status. Esp. considering the following transposition
> report() -> my_error() that I am planning for BUG#25597 still
> to be discussed with you and team.

OK.

> I'll be back on Aug 11th to finish up this bug.
> 
> Enjoy your summer time!

Thanks! You too!

Best wishes,
Mats Kindahl
-- 
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems