Re: bk commit into 5.1 tree (aelkin:1.2612) BUG#36524

[Andrei Elkin]

Mats, hello.

Here is a new patch

http://lists.mysql.com/commits/77444

that follows principals we agreed on.

Answering,

>>>
>>> Suppose that the function is used in this way somewhere, maybe by mistake,
> maybe
>>> intentional, with a new error code ER_DEADLOCK_IS_FATAL:
>>>
>>> /*
>>>    If we get a deadlock here, this is to be reported as an error.
>>>  */
>>> int error= thd->stmt_da->sql_errno();
>>> if (error == ER_LOCK_DEADLOCK)
>>>   thd->report(ERROR_LEVEL, ER_DEADLOCK_IS_FATAL, "A fatal deadlock
> occured");
>> 
>> That would be an incorrect use.
>> What's a point of translating a temporary error into an ordinary one?
>> The ordinary error would be induced by a temporary, but it's not equal
>> to the temporary.
>> Therefore the correct way to achive the goal is to report the ordinary,
>> like ER_DEADLOCK_IS_FATAL, not instead but afterward.
>> And if one *really* needs translation he has to *remove* all traces of 
>> the temporary, incl ones from the diagnostics area, and to report the ordinary
>> on a "clean sheet".
>
> No objections there, but that means that the interface, basically the report()
> function, needs to be changed so that the above cannot be done by "normal" mistakes.
>

That's right. I had to changed report() in two ways.
1. it behaves now basically as a shortcut of my_error() and sql_print_error() 
   hence allowing multiple invocations per statement
2. a new, i called it, display() method harvests errors to store them in 
   the show slave status area, as report() was used to do.
   This happens once a slave thread breaks its operational loop.

I have not made the method to accept THD as an argument though thinking that
it'd be better to move the methods into Relay_Log_Info and Master_Info instead
if it will come to eliminate current_thd references (which I am okay 
performance-minding as well: error handling is normally not supposed to be optimal).

>>> Now, since the patch demotes the error to a warning inside report(), this
> will
>>> cause the function above to not do what was intended because the calling
> code
>>> thinks this is an error (ER_DEADLOCK_IS_FATAL) while report() thinks this is
> a
>>> warning (ER_LOCK_DEADLOCK). This occurs because has_temporary_error() talks
>>> about the error stored in the diagnostics area (ER_LOCK_DEADLOCK) while the
>>> calling code talks about the code passed to the report() function
>>> (ER_DEADLOCK_IS_FATAL).
>>>
>>> From my point of view, you can solve this either by:
>>>
>>> - Convincing me that this cannot happen, i.e.: that nobody currently does or
>>>   ever wants to do anything like the above.
>> 
>> I believe it can not happen while we stick to some sensible principle.
>> One is suggest above: don't do tricks instead report new error.
>
> Right, so we want to eliminate the possibility of doing tricks like the above.
>

So we are having proposal to stick to rli->report() and rli->display(),
resembling my_error() and send_error() of the plain user connection.
The scheme would guarantee no abuse of the demotion as well: report() only accumulates 
never over-writes.


>>> - Changing the patch to ensure that has_temporary_error() and calling code
> talks
>>>   about the same error code either by:
>>>
>>>   - Passing the error code passed to report() further on to the
>>>     has_temporary_error() function when doing the check
>>>
>> 
>>>   - Removing the error code from the parameter list of the report() function
> and
>>>     reading the error code from the diagnostics area inside the report()
>>>     function
>> 
>> Yes, the 2nd is something I did not make but liked.
>> 
>> Passing the last error through da does not make the convention of not doing
> tricks
>> irrevant. But it would make that activity exposed more so that reviewers hardly
> will
>> miss the chance to ask about.
>> 
>> I believe we are on the same page now and I am submitting the da-error-trasport
> patch
>> in a short time.
>
> Sounds excellent! I'm eagerly awaiting a patch.

The changes in the new patch covers part of remained issues of BUG#25597 as well.
There are about 28 changed results files I did not include with the patch to let us
concentrate on essential things.


cheers,

Andrei