Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3117) Bug#47327

[Luís Soares]

Hi Alfranio,

On Fri, 2009-11-06 at 16:50 +0000, Alfranio Correia wrote:
> Hi Luis,
> 
> Thank you.
> 
> See some comments in-line.
> 
> Cheers.
> 
> Luís Soares wrote:
> > Hi Alfranio, 
> > 
> >   Please find my review comments below. Let me know If I
> > misinterpreted the purpose of the patch.
> > 
> > Regards,
> > Luís
> > 
> > STATUS
> > ------
> >  
> >   Not approved.
> > 
> > REQUIRED CHANGES
> > ----------------
> >   
> >   R1. Maybe it's just me, but I find the commit message
> >       unclear. I think there is different behavior for STMT and
> >       MIXED/ROW when N-statements are involved and this should be
> >       clear. Can you please rewrite it?
> > 
> >   R2.
> >       As I read it there are two problems reported:
> > 
> >          1. MIXED/ROW: T-statements are not truncated even though
> >             they could be (they are written to the transactional
> >             cache and binlogging is deferred until transaction
> >             COMMIT/ROLLBACK. OTOH, N-statements are written to

Just some correction here: "... and binlogging is deferred until
transaction COMMIT."

> >             the non-transactional cache and binlogged upon
> >             statement COMMIT/ROLLBACK. Therefore, we can clear
> >             transactional cache without affecting N-statements
> >             logging).
> > 
> >          2. STMT: T-statements and N-statements are written to
> >             the transactional cache and are both deferred until
> >             transaction COMMIT/ROLLBACK. However, if a SAVEPOINT
> >             is set, there are only T-statements *after* the
> >             savepoint and a ROLLBACK to the savepoint is issued,
> >             then the transactional changes are not removed from
> >             cache back until the savepoint.
> > 
> >          After investigating a bit, I came up with the following
> >          test case, that showed me that this patch does not
> >          address issue #2. Please check the some comments inline
> >          the test case.
> 
> 
> I will look at the messages above and see if they make sense.

Ok.

> > 
> > ==== TEST CASE
> > 
> > -- source include/have_log_bin.inc
> > -- source include/have_innodb.inc
> > 
> > CREATE TABLE nt (a int);
> > CREATE TABLE tt (a int) engine=InnoDB;
> > 
> > SET AUTOCOMMIT=0;
> > 
> > # case #1
> > BEGIN;
> > 
> > INSERT INTO tt VALUES (1);
> > INSERT INTO nt VALUES (2);
> > 
> > SAVEPOINT s1;
> > 
> > INSERT INTO tt VALUES (3)
> > INSERT INTO nt VALUES (4);
> > 
> > ROLLBACK TO SAVEPOINT s1;
> > 
> > COMMIT;
> > 
> > #
> > #  ROW: logging of INSERT 3 should *not* be done
> > #  STMT: logging of INSERT 3 and INSERT 4 *should* be done.
> > #
> > 
> > #### Your patch fixes the above case (case #1) for ROW and STMT
> > 
> > # case #2
> > BEGIN;
> > 
> > INSERT INTO tt VALUES (10);
> > INSERT INTO nt VALUES (20);
> > 
> > SAVEPOINT s1;
> > 
> > INSERT INTO tt VALUES (30);
> > 
> > ROLLBACK TO SAVEPOINT s1;
> > 
> > COMMIT;
> > 
> > #
> > # ROW: logging of INSERT 30 should *not* be done
> > # STMT: logging of INSERT 30 should *not* be done
> > #
> > 
> > -- exit
> > 
> > #### Your patch *does not* fix the above for STMT.
> 
> 
> ok. It is missing a condition in the rollback.

Great that we found this.

> > 
> > ==== END OF TEST CASE
> > 
> >     I think that case #2 is not a big deal, but should be fixed
> >     as part of part of this bug report, wouldn't you say?
> > 
> >   R3. Please, to make bazaar bundles useful, don't commit on top
> >       of your own local revisions. I tried to merge the bundle on
> >       a vanilla mysql-5.1-rep+3 latest tree and it failed:
> 
> This is strange because I've used the branch pushed to rep+3. So
> they are supposed to be equal.
> 

Dunno what happened... You can always try it yourself and check what
happened. :)

Cheers,
Luís

> > 
> >        "bzr: ERROR: Your branch does not have all of the revisions
> > required in order to merge this merge directive and the target location
> > specified in the merge directive is not a branch:
> >
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-47327/mysql-5.1-rep%2B3/"
> > 
> >>  }
> >>  
> >> @@ -1818,12 +1863,15 @@ static int binlog_savepoint_rollback(han
> >>  {
> >>    DBUG_ENTER("binlog_savepoint_rollback");
> >>  
> >> +  binlog_cache_mngr *const cache_mngr=
> >> +    (binlog_cache_mngr*) thd_get_ha_data(thd, binlog_hton);
> >> +
> >>    /*
> >>      Write ROLLBACK TO SAVEPOINT to the binlog cache if we have updated
> some
> >>      non-transactional table. Otherwise, truncate the binlog cache starting
> >>      from the SAVEPOINT command.
> >>    */
> >> -  if (unlikely(thd->transaction.all.modified_non_trans_table || 
> >> +  if (unlikely(cache_mngr->is_stmt_committed() ||
> >>                 (thd->options & OPTION_KEEP_LOG)))
> >>    {
> >>      int errcode= query_error_code(thd, thd->killed == THD::NOT_KILLED);
> 
> It is missing cache_mngr->is_stmt_committed() &&
> cache_mngr->get_pos_stmt_committed() > save_point
> 
> 
> >> @@ -4276,6 +4324,9 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
> >>        {
> >>          file= &cache_mngr->trx_cache.cache_log;
> >>          cache_data= &cache_mngr->trx_cache;
> >> +        if (thd->variables.binlog_format == BINLOG_FORMAT_STMT
> &&
> >> +            thd->transaction.stmt.modified_non_trans_table)
> >> +          cache_mngr->calc_pos_stmt_committed();
> >>        }
> >>  
> >>        thd->binlog_start_trans_and_stmt();
> >>
> 
-- 
Luís