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
> 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.
>
> ==== 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.
>
> ==== 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.
>
> "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();
>>
