List:Commits« Previous MessageNext Message »
From:Luís Soares Date:January 29 2010 10:19am
Subject:Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)
Bug#50157
View as plain text  
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



Thread
bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127) Bug#50157Li-Bing.Song29 Jan
  • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3127)Bug#50157Luís Soares29 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#50157Libing Song30 Jan