List:Commits« Previous MessageNext Message »
From:Libing Song Date:January 29 2010 7:01am
Subject:Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)
Bug#50157
View as plain text  
Hi Luis,
Thanks for your review. The new patch has been committed, please review
again and find my comments below.

On Thu, 2010-01-28 at 14:59 +0000, Luís Soares wrote:

> REQUESTS
> --------
> 
>   R1. Let me see if I got this straight. This is about memory
>       management. You use the following check you use in
>       free_nodes_before:
> 
>         ((&(block->nodes[0]) <= node) && 
>          (&(block->nodes[BLOCK_TRANX_NODES]) >= node))
> 
>       If a transaction requires more than BLOCK_TRANX_NODES, then
>       it needs two blocks. If at that time,
>       free_nodes_before(nodeY) gets called, and we are in the
>       following scenario:
>  
>         1. nodeY is logically after nodeX;
>         2. &nodeY is in the second block;
>         3. and malloc has assigned the memory for this second
>            block from some chunk that was freed before and was
>            lying around in its list of free chunks (this is a
>            possible implementation of malloc - glibc uses it I
>            think);
>         4. and the first block was not taken from the list of
>            free memory chunks (meaning that it required a call to
>            brk, for instance).
> 
>       Wouldn't this mean that for nodeY (which is logically after
>       nodeX), would be considered before nodeX? Wouldn't this
>       mess things up?
Yes, The first Block's address cat be larger than the second and nodeY
can be considered before nodeX.
But We only use the code to find the Block containing the given node
nodeY. All blocks before the block containing nodeY are freed. Even
though nodeX's address is after nodeY's, it can be freed. For nodeX's
Block is before nodeY's Block.
> 
> SUGGESTIONS
> -----------
> 
>   S1. In sql/rpl_handler.cc, maybe we could avoid the changes
>       regarding the initialization for Trans_param if we have a
>       default constructor for the structure in sql/replication.h
> 
>        typedef struct Trans_param {
> 
>          (...)
> 
>          Trans_param() : server_id(0), 
>            flags(0), log_file(NULL), log_pos(0) {};
It is the API of plugins, so we have to keep a pure 'C' structure,
For a plugin can be compiled by the 'C' compilers.
> 
>          (...)
> 
>        } Trans_param;
> 
>       I think that, yet another different approach, with less
>       changes, would be:
> 
>         -  Trans_param param;
>         +  Trans_param param= {0};
I had intended to code like this at first. But some warnings are produced, 
so I gave it up.

"rpl_handler.cc:191: warning: missing initializer for member
‘Trans_param::flags’"

>      Have you considered? Your changes are OK, just asking
>      because in most places three out of four Trans_param members
>      are initialized and only if FOREACH_OBSERVER is called the
>      server_id member is initialized... Just feels like a loose
>      end... Anyway, nothing to be worried about... Your changes
>      are also fine. I leave it up to your consideration!
> 
> DETAILS 
> -------
>  See more comments inline.



-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer
Certified (ISC)2 CISSP

Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================


Thread
bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127) Bug#50157Li-Bing.Song27 Jan
  • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157He Zhenxing28 Jan
    • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157Libing Song29 Jan
      • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157He Zhenxing29 Jan
  • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157Luís Soares28 Jan
    • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157Libing Song29 Jan
      • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157Luís Soares29 Jan