Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259

[Rafal Somla]

Hi Ingo,

All is clear now - thanks for putting effort in explaing it so well. I've 
approved the patch.

Ingo Strüwing wrote:
>> 1. I still don't understand a piece of the code. See the remarks about
>> this comment: "/* If HIT_LIMIT is 1, return error to suppress send_ok().
>> */" and the following code.
> 
> Does the following make a better explanation?
> 
>   /* Execute the special sync point 'now' if activated above. */
>   if (is_dsp_now)
>   {
>     DEBUG_SYNC(thd, "now");
>     /*
>       If HIT_LIMIT for sync point "now" was 1, the execution of the sync
>       point decremented it to 0. In this case the following happened:
> 
>       - an error message was reported with my_error() and
>       - the statement was killed with thd->killed= THD::KILL_QUERY.
> 
>       If a statement reports an error, it must not call send_ok().
>       The calling functions will not call send_ok(), if we return TRUE
>       from this function.
> 
>       To detect, if an error message has been reported, we can check one
>       of the above conditions. That is, either examine the error message
>       stack for an entry, or check thd->killed. We do the latter below.
>     */
>     if (thd->killed)
>       DBUG_RETURN(TRUE);
>   }
> 

Yes, now even I understand what is going on here :)

> 
>> 2. About the st_debug_sync_action structure which is moved around with
>> memmove: for safety I propose to add a comment explaining potential
>> problems with constructors/destructors.
> 
> I changed the declaration to:
> 
> /*
>   Action to perform at a synchronization point.
>   NOTE: This structure is moved around in memory by realloc(), qsort(),
>         and memmove(). Do not add objects with non-trivial constuctors
>         or destructors, which might prevent moving of this structure
>         with these functions.
> */

Perfect!

>> How about such compromise: add comment to the documentation of
>> st_debug_sync_action structure saying that it is moved around using
>> memmove and therefore can not contain members which have non-trivial
>> constructors or destructors.
> 
> I hope I addressed it sufficiently with my comment near the top of this
> email.
>

Yes, absolutely.

>> I don't see why you check thd->killed here and how it relates to
>> HIT_LIMIT...
> 
> I hope I addressed it sufficiently with my comment near the top of this
> email.
>

Yes, indeed.

>> Sure, that is enough, except... that there is @see doxygen keyword which
>> would be good to use here so that a reader of doxygen docs can click on it.
> 
> Is this better?
> 
>   @see the function comment of debug_sync_token() for more constraints
>     for the string.
>

Yes, I think so - I'm not 100% sure how doxygen handles the @see keyword. But 
your comment is definitely good enough.

Rafal