List:Commits« Previous MessageNext Message »
From:Libing Song Date:August 23 2010 2:41pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)
Bug#39804
View as plain text  
Hi andrei,
Thank for your review. Please find my comments below.

On Mon, 2010-08-23 at 17:12 +0300, Andrei Elkin wrote:
> Li-Bing, hello.
> 
> The patch looks good. Thanks for accepting binlogging merge in
> select_insert::abort_result_set().
> After thinking little bit more I have gathered couple of notes.
> 
> > #At file:///home/anders/work/bzrwork1/wt1/mysql-next-mr-bugfixing/ based on
> revid:bernt.johnsen@stripped
> >
> >  3227 Li-Bing.Song@stripped	2010-08-23
> >       Bug #39804 Failing CREATE ... SELECT doesn't appear in binary log.
> >       
> >       Failing CREATE ... SELECT that updates non-transactional tables
> >       via stored function is not binlogged on SBR. The cause is that the
> >       statement will never be binlogged on SBR if it fails. As the table
> >       will be dropt automatically after an error happens.
> >             
> >       In this patch, the statement will be binlogged on SBR if a other
> >       non-trasactional table is modified.
> >      @ mysql-test/include/rpl_diff_tables.inc
> >         Always switches to master at the end.
> >      @ mysql-test/suite/rpl/t/rpl_create_table.test
> >         Add test for bug#39804
> >      @ sql/sql_class.h
> >         Add modified_other_non_trans_table into select_create class.
> >      @ sql/sql_insert.cc
> >         Binlog the CTS statement if other non-trasactional tables is modified.
> 
> >         We cannot tmp_disable_binlog() and always set 
> >         thd->transaction.stmt.modified_non_trans_table FALSE. 
> 
>    Disabling in the base code was unconditional. That is binlog file
>    never containted a failed C..S.
>    This bug reveals a case when binlogging has to be done. That
>    indicates tmp_disable_binlog(thd) should not be in the exec path.
ok.
>    That is (see continuation on @@ -3924,10 +3966,8 @@ void
>    select_create::abort_result_set() hunk) ...
> 
> >         As we need binlog
> >         the statement on some occasions.
> >
> >
[snip]
> >  
> >    DBUG_EXECUTE_IF("sleep_create_select_before_check_if_exists",
> my_sleep(6000000););
> > @@ -3910,7 +3932,27 @@ void select_create::abort_result_set()
> >    DBUG_ENTER("select_create::abort_result_set");
> >  
> >    /*
> > -    In select_insert::abort() we roll back the statement, including
> > +    In RBR, non-transactional table's row events are never put in the
> > +    transactional cache. So stmt.modified_non_trans_table is set to
> > +    FALSE and the transactional cache should be truncated.
> > +
> > +    In SBR, the statement should be binlogged only when there is some
> > +    non-transactional tables are modified by SELECT clause.
> > +
> 
> > +    SELECT clause executes before creating the table. So it is possable
> > +    that an error encounters before creating the table and
> > +    modified_by_select_non_trans_table is not set at the moment.
> > +    Fortunatally, if stmt.modified_non_trans_table is set, it implies
> > +    some other non-transactional tables are modified. So we just keep
> > +    the original value of stmt.modified_non_trans_table.
> 
> May I offer you the following text of comments
> 
>       SELECT clause executes before creating the table and can modify a
>       non-transactional table. The current statement has to inherit
>       `modified_non_trans_table' of the preceeding select in order to
>       binlog correcty.
>       And it does so through recording the select's value in 
>       select_create::prepare to restore it here:
OK.
> 
>       if (table)
>          thd->transaction.stmt.modified_non_trans_table=
>          modified_by_select_non_trans_table;
> 
> > +  */
> > +  if (table)
> > +    thd->transaction.stmt.modified_non_trans_table=
> > +      (thd->is_current_stmt_binlog_format_row() ?
> > +       FALSE : modified_by_select_non_trans_table);
> 
> I don't see any need to complicate the assignment with 
> thd->is_current_stmt_binlog_format_row().
> The idea of `modified_non_trans_table' stands separately to
> the binlog format.
Please see the comments.
> +    In RBR, non-transactional table's row events are never put in the
> +    transactional cache. So stmt.modified_non_trans_table is set to
> +    FALSE and the transactional cache should be truncated.
On 5.5, all row events for non-transactional tables are never written
into trans cache. So there are only row events for transactional tables
in trans cache. It implies that an error encounters if abort_result_set
is called and the transaction will be rolled back. All row events in
trans cache are on transactional tables so the whole trans cache(include
the 'CREATE TABLE' statement) should be truncated and not be binlogged.
It will cause bug#55625, if the cache is binlogged. In RBR, we always
pretend there is no non-transactional table modified and the cache is
always truncated.
> 
> 
> > +
> > +  /*
> > +    In select_insert::abort_result_set() we roll back the statement, including
> >      truncating the transaction cache of the binary log. To do this, we
> >      pretend that the statement is transactional, even though it might
> >      be the case that it was not.
> 
> > @@ -3924,10 +3966,8 @@ void select_create::abort_result_set()
> >      of the table succeeded or not, since we need to reset the binary
> >      log state.
> >    */
> 
> ... (continuing)
> 
> > -  tmp_disable_binlog(thd);
> 
> 
>    if (!modified_by_select_non_trans_table)
>       tmp_disable_binlog(thd);
First, We cannot use modified_by_select_non_trans_table here, it will
not be set if an error encounters before prepare() is called.
Second, 'thd->transaction.stmt.modified_non_trans_table= FALSE' has the
same effect with it.
So I think tmp_disable_binlog() is not really necessary.
> 
> to indicate the bug's real binlogging-at-abort case.
> 
> >    select_insert::abort_result_set();
> > -  thd->transaction.stmt.modified_non_trans_table= FALSE;
> > -  reenable_binlog(thd);
> 
> Similarly to reenable_binlog().
> 
> > +
> >    /* possible error of writing binary log is ignored deliberately */
> >    (void) thd->binlog_flush_pending_rows_event(TRUE, TRUE);
> >  
> >
> 
> cheers,
> 
> Andrei

-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================

Thread
bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227) Bug#39804Li-Bing.Song23 Aug
Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)Bug#39804Libing Song23 Aug
Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)Bug#39804Libing Song24 Aug
Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)Bug#39804Libing Song25 Aug