List:Commits« Previous MessageNext Message »
From:Luís Soares Date:January 29 2010 9:28am
Subject:Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)
Bug#50157
View as plain text  
Hi Libing,

On Fri, 2010-01-29 at 15:01 +0800, Libing Song wrote:
> 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.

Aha, this is clear now.
So ordering between blocks (and therefore between cluster 
of nodes) is not important in this case... I get it.


> > 
> > 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.

OK, got it.

> > 
> >          (...)
> > 
> >        } 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’"

Right! So leave it as it is!

Still it's kind of ironic that we get a warning when we
do this, but not when we initialize the structure members
one by one.

Thanks for clearing my mind.

I'll have a look at the new patch now.

Regards,
Luís


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