Hi Libing,
Approved.
Please find some minor corrections below (mostly just typos).
Regards,
Luís
On Fri, 2010-01-29 at 06:39 +0000, Li-Bing.Song@stripped wrote:
> #At file:///home/anders/work/bzrroot/mysql-5.1-rep-semisync/ based on
> revid:zhenxing.he@stripped
>
> 3127 Li-Bing.Song@stripped 2010-01-29
> BUG#50157 Assertion !active_tranxs_->is_tranx_end_pos(..) in
> ReplSemiSyncMaster::commitTrx
>
> The root cause of the crash is that a TranxNode is freed before it is used.
> A TranxNode is allocated and insertted into the active list each time
typo: insertted --> inserted
> a log events is written and flushed into the binlog file.
typo: log events --> log event
> The memory for TranxNode is allocated with thd_alloc and will be freed
> at the end of the statement. The after_commit/after_rollback callback
> was supposed to be called before the end of each statement and remove the node
> from
> the active list. However this assumption is not correct in all cases(e.g. call
>
> 'CREATE TEMPORARY TABLE myisam_t SELECT * FROM innodb_t' in a transaction
> and delete all temporary tables automatically when a session closed),
> and can cause the memory allocated for TranxNode be freed
> before it was removed from the active list. So The TranxNode pointer in the
> active
> list would become a wild pointer and cause the crash.
>
> After this patch, We had a class called a TranxNodeAllocate which manages the
> memory
typo: We had --> we have
> for allocating and freeing TranxNode. It uses my_malloc to allocate memory.
> @ sql/rpl_handler.cc
> params are not initialized.
[snip]
>
> #include "semisync.h"
>
> +struct TranxNode {
> + char log_name_[FN_REFLEN];
> + my_off_t log_pos_;
> + struct TranxNode *next_; /* the next node in the sorted list */
> + struct TranxNode *hash_next_; /* the next node during hash collision */
> +};
> +
> +/**
> + @class TranxNodeAllocator
> +
> + This class provides memory allocating and freeing methods for
> + TranxNode. The main target is performance.
> +
> + <h3>Block</h3><p>
> + A sequence memory which contains BLOCK_TRANX_NODES TranxNodes.
> +
> + <h3>BLOCK_TRANX_NODES</h3><p>
> + The number of TranxNodes which are in a Block.
> +
> + <h3>Block Link Table</h3><p>
> + Every Block has a 'next' pointer which points to the next Block. These
> + linking Blocks constitut a Block link table. The 'first_block' is the
typo: constitut --> constitute
> + head, and the 'last_block' is the rear.
> +
> + <h3>current_block</h3><p>
> + current_block is always points the Block in the Block link table in
typo: current_block is always --> current_block always
> + which the last allocated node is. The Blocks before it are all in use
> + and the Blocks after it are all free.
> +
> + <h3>last_node</h3><p>
> + It always points to the last node which has been allocated in the
> + current_block.
> +
> + <h3>How to allocate a node</h3><p>
> + The pointer of the first node after 'last_node' in current_block is
> + returned. current_block will move to the next free Block when all nodes of
> + it are in use. A new Block is allocated and is put into the rear of the
> + Block link table if no Block is free.<p>
> +
> + The list starts up empty (ie, there is no allocated Block).<p>
> +
> + After some nodes are freed, there probably are some free nodes before
> + the sequence of the allocated nodes, but we do not reuse it. It is better
> + to keep the allocated nodes are in the sequence, for it is more efficient
> + for allocating and freeing TranxNode.
> +
> + <h3>How to free nodes</h3><p>
> + There are two methods for freeing nodes. They are free_all_nodes and
> + free_nodes_before.<p>
suggestion: Maybe I would put "free_nodes_before (TranxNode *node)"
as we reference the "given node" further down the text.
> +
> + 'A Block is free' means all of its nodes are free.<p>
> + As all allcoated nodes are in the sequence, 'Before one node' means all
typo: allcoated
> + nodes before given node in the same Block and all Blocks before the Block
> + which containing the given node. As such, all Blocks before the given one
> + ('node') are free Block and moved into the rear of the Block link table.
> + The Block containing the given 'node', however, is not. For at least the
> + given 'node' is still in use. This will waste at most one Block, but it is
> + more efficient.<p>
> +
> + <h3>How to free Blocks</h3><p>
> + If there are some free Blocks and the total number of the Blocks in the
> + Block link table is larger than the 'reserved_blocks', Some free Blocks
> + will be freed until the total number of the Blocks is equal to the
> + 'reserved_blocks' or there is only one free Block behind the
> + 'current_block'.
> + */
> +#define BLOCK_TRANX_NODES 16
> +class TranxNodeAllocator
> +{
[snip]
> +
> + /**
> + All Blocks before the given 'node' are free Block and moved into the rear
> + of the Block link table.
> +
> + @param node All nodes before 'node' will be freed
> +
> + @return Return 0, or 1 if an error occoured.
typo: occoured
Regards,
Luís
