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
