List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:January 25 2010 3:02am
Subject:Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3128)
View as plain text  
Hi Libing,

Thank you for the work!

Not Approved!

RC1. the patch does not compile with gcc-4.3.2, the problem is that
expressions like:
    block->end = block->begin + block_size_;
    last_node_ += sizeof(T);
    last_node_ -= sizeof(T);
uses void* pointers in arithmetic, please considering using char*.

RC2. this patch does not free the blocks allocated, because we always
allocate the nodes in ascend order, and free the nodes in descend order,
I think you can free all blocks that all nodes are before the smallest
binlog position.

RC3. Please don't do this in a real test file:
     call mtr.add_suppression(".*");
    This will suppress all warnings.

R1. I think C++ templates can be avoided for NodeAllocator, because this
will only be used in semisync and for TranxNode, and I'd also suggest
name it TranxNodeAllocator to make this clear.

R2. Please add the class NodeAllocator to semisync_master.h

R3. Please put the test in rpl_semi_sync.test


Please also look below for some in line comments.

Li-Bing.Song@stripped wrote: 
> #At file:///home/anders/work/bzrroot/mysql-5.1-rep-semisync/ based on
>  3128 Li-Bing.Song@stripped	2010-01-22
>       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 created when each time some log events are written into binlog
> file

A TranxNode is allocated and inserted each time when ....

>       and is synchronized. TranxNodes' memories are allocted from mem_root of the
> current
>       thread, and will be freed immediately after current statement ending.

and is flushed. The memory for TranxNode is allocated with thd_alloc,
which will be freed at the end of the statement. The
after_commit/rollback callback was supposed to be called before the
ending of each statement and remove the node from the active list.
However, this assumption is not correct in all cases (e.g. CREATE
TEMPORARY TABLE ... SELECT), and can cause the memory allocated for
TranxNode been freed before it was removed from the active list.

>       from TranxNode list. So the Pointer of the TranxNode in TranxNode list becomes
> a 
>       wild pointer.

So the TranxNode pointer in the active list would become a wild pointer
and cause the crash.

>       After this patch, TranxNodes are not allocated from mem_root.

After this patch, the memory for TranxNode will be allocated by

[ snip ]

> === added file 'plugin/semisync/semisync_node_allocator.h'
> --- a/plugin/semisync/semisync_node_allocator.h	1970-01-01 00:00:00 +0000
> +++ b/plugin/semisync/semisync_node_allocator.h	2010-01-22 14:59:03 +0000
> @@ -0,0 +1,174 @@
> +/* Copyright (C) 2010 MySQL AB
> +

RC4. Newly created file should have the copyright:

Copyright (C) 2010 Sun Microsystems.

[ snip ]

bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3128) Bug#50157Li-Bing.Song22 Jan
  • Re: bzr commit into mysql-5.1-rep-semisync branch (Li-Bing.Song:3128)Bug#50157He Zhenxing25 Jan