Andrei Elkin wrote:
> Mats, hej.
>
>
>
>> Hi Andrei!
>>
>> Patch is OK to push, but please read my comments below.
>>
>>
>
>
[snip]
>> This comment is close to unreadable in Doxygen. Check the Doxygen
>> output later, and fix the comment. Not necessary to do now, however.
>>
>>
>
> Okay. I'd love to improve them but I really got a hard time trying to doxygenize.
> Using intructions on https://inside.mysql.com/wiki/Doxygen
> did not help me much.
>
> Do you know by chance some other source of how to do it?
>
I'm actually reading the manual mostly.
Basically, the following template should be sufficient:
/**
<Brief description>.
<Full description>
@param param1 Whatever
@param param2 Whatever
@retval 0 No error
@retval ER_GOOFY Something is goofy
*/
[snip]
>>> @@ -6288,8 +6311,9 @@ int Rows_log_event::do_apply_event(Relay
>>> if (!get_flags(COMPLETE_ROWS_F))
>>> bitmap_intersect(table->write_set,&m_cols);
>>> + this->slave_exec_mode= slave_exec_mode_options; // fix the
>>> mode
>>> +
>>>
>>>
>> Is this really the right place to put that update? I suspect that it
>> is better to pass that parameter as part of the execution context,
>> i.e., either as a parameter or in Relay_log_info structure.
>>
>> In general, using member variables for parameter passing leads to very
>> big objects, and also increases the size of the code as well as kills
>> a lot of optimizations.
>>
>
> It's hard to disagree with the last statement.
> However, the same would apply to rli object...
>
> There were several reasons why Rows_log_event was choosen:
>
> - avoiding messing with concurrency:
> a rli instance is shared between many threads
>
This can be handled by passing it as a parameter (or use a mutex for the
RLI, which I think is already done).
> - the mode is to stay unchanged at least for the entire time of
> executing of the event so that its external altering won't affect
> the current event handling;
>
This can be handled by passing it as a parameter.
> - no hassles with signature change:
> rli is not always available in the Rows_log_event's methods;
>
Yes, you would have to change the signature whatever you do, but I see
this as a minor problem as long as it is in internal functions and not
part of an interface.
[snip]
>>> - if ((keynum= table->file->get_dup_key(error)) < 0)
>>> + if (error == HA_ERR_LOCK_DEADLOCK ||
>>> + error == HA_ERR_LOCK_WAIT_TIMEOUT ||
>>> + (keynum= table->file->get_dup_key(error)) < 0 ||
>>> + !overwrite)
>>>
>>>
>> Wouldn't it be better to check the errors for which get_dup_key()
>> behaves correctly?
>>
>
> You meant that if the error in
>
> while ((error= table->file->ha_write_row()
>
> is set to HA_ERR_FOUND_DUPP_UNIQUE or HA_ERR_FOUND_DUPP_KEY are the
> only warrants to go into the body of the loop, did not you?
>
> I think that will make sense esp if yet another handler error will be issued.
>
Yes, that is what I meant.
>
>> As it stands now, this could would have to be
>> changed if a HA_ERR_LOCK_LIVELOCK was introduced.
>>
>
> What is the reason to have that?
>
Just an example of an error that would require the checks to change
(since you wouldn't want to call the get_dup_key() in this case for the
same reasons that you wouldn't want to call it when there's an
HA_ERR_LOCK_DEADLOCK).
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
