MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:April 15 2010 11:26am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch
(alfranio.correia:3155) Bug#51894 Bug#52616
View as plain text  
Hi Jasonh,

Thank you for your review.
See my comments in-line.

Cheers.

He Zhenxing wrote:
> Hi Alfranio,
> 
> I think the code is OK, Please look for some comments below!
> 
> STATUS
> ------
> Not Approved!
> 
> REQUESTS
> --------
> R1. There are some comments about that DROP TEMPORARY TABLE not binloged
> in ROW format, which should be fixed.

ok. I missed that.

> 
> R2. I think probably we need a new unsafe ER code for CREATE/DROP
> TEMPORARY TABLE. Currently the reason for unsafeness is:
> Non-transactional reads or writes are unsafe if they occur, Which I
> think is misleading.

I thought about that but decide not to do it in the last
minute. Note that the unsafe cases happen only when there is a mixed
statement: non-transactional & temporary table. BUG#51564 introduces
a new ER code for this case.

So. If you don't mind I will keep the current code and use the ER code
introduced by BUG#51564.

> 
> R3. I don't quite understand the code to calculate 'mixed_engine',
> especially why to check SQLCOM_CREATE_TABLE. And I think the following
> line is not correct:

Note that the unsafe cases happen only when there is a mixed
statement: non-transactional & temporary table.

So CREATE TEMPORARY TABLE t_temp SELECT * FROM t_mysiam is one
case.

> 
>  mixed_engine= mixed_engine || (!prev_vote || !act_vote)

This is right.
ok. changed.

> 
> I think it should be:
> 
>  mixed_engine= mixed_engine || (prev_vote != act_vote)

Both are equivalent but your proposal is clearly better.
ok. done.

> 
> And I'm thinking the following logic is clearer:
> 
> if (!all_trans_engines && !mixed_engine)
> {
>   mixed_engine= table->file->has_transactions() ||
>                 table->table->s->tmp_table;
> }
> 
> The idea is that if all_trans_engine is true, then mixed_engine cannot
> be true, if mixed_engine is already true, then no need to calculate any
> more. otherwise all previous tables are non-trans, and mixed_engine will
> be true if current table is trans or tmp.

This is not right. Note that the all_trans_engines is set when the table
is updated and the mixed_engine for both reads and writes.
Patch BUG#49019 changes the name of the variables and adds some comments
explaining their usage.

> 
> SUGGESTIONS
> -----------
> S1. Since you have already add a detailed description of the problem and
> solution on the bug report, I'd suggest to avoid long details and
> examples in the commit messages, because the commit messages will show
> up in the bug report every time the patch is committed, which kind of
> flooding the report page :) Please look below!

ok. done.

> 
> S2. reset_current_stmt_binlog_format_row(), I'd suggest:
> 
> if (!in_sub_stmt)
> {
>   if (variables.binlog_format == BINLOG_FORMAT_ROW)
>     set_current_stmt_binlog_format_row();
>   else if (!temporary_tables)
>     clear_current_stmt_binlog_format_row();
> }

ok. done.

> 
> Please also look below for some in-line comments.
> 
> Alfranio Correia wrote:
>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-51894/mysql-next-mr-bugfixing/
> based on revid:marc.alff@stripped
>>
>>  3155 Alfranio Correia	2010-04-14
>>       BUG#51894 Replication failure with SBR on DROP TEMPORARY TABLE inside a
>>                 transaction
>>       BUG#52616 Temp table prevents switch binlog format from STATEMENT to ROW
>>       
>>       Before the WL#2687 and BUG#46364, every non-transactional change that
> happened
>>       after a transactional change was written to trx-cache and flushed upon
>>       committing the transaction. WL#2687 and BUG#46364 changed this behavior
> and
>>       non-transactional changes are now written to the binary log upon
> committing
>>       the statement.
>>       
>>       A binary log event is identified as transactional or non-transactional
> through
>>       a flag in the Log_event which is set taking into account the underlie
> storage
>>       engine on what it is stems from. In the current bug, this flag was not
> being
>>       set properly when the DROP TEMPORARY TABLE was executed, e.g.:
>>       
>>       --- sql/sql_table.cc    2010-03-02 14:34:50 +0000
>>       +++ sql/sql_table.cc    2010-03-10 01:39:40 +0000
>>                  tables).  In this case, we can write the original query into
>>                  the binary log.
>>                 */
>>       -        error |= write_bin_log(thd, !error, thd->query(),
> thd->query_length());
>>       +        error |= write_bin_log(thd, !error, thd->query(),
> thd->query_length(),
>>       +                               ---> TRUE if innodb, FALSE if MyIsam
> <---);
>>              }
>>              else if (thd->is_current_stmt_binlog_format_row() &&
>>                       tmp_table_deleted)
>>       
> 
> I'd suggest remove the above code fragment.
> 
>>       However, while fixing this bug we figured out that changes to temporary
> tables
>>       should be always written to the trx-cache if there is an on-going
> transaction.
>>       This is necessary because
>>       
>>       BEGIN;
>>       INSERT INTO t_innodb SELECT * FROM t_myisam_temp; /* M-Statement */
>>       DROP TEMPORARY TABLE t_myisam_temp;
>>       COMMIT;
>>       
>>       would produce the following events into the binary log
>>       
>>       BEGIN;
>>       DROP TEMPORARY TABLE t_myisam_temp;
>>       COMMIT;
>>       
>>       BEGIN;
>>       INSERT INTO t_innodb SELECT * FROM t_myisam_temp; /* ---> ERROR <---
> */
>>       COMMIT;
>>       
> 
> I'd suggest to simplify the above as something like:
> 
> would produce binlog events in the reversed order.
> 
>>       Regarding concurrency, keeping changes to temporary tables in the trx-cache
> is
>>       also safe as temporary tables are only visible to the owner connection.
>>       
>>       In this patch, we classify the following statements as unsafe:
>>          1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>       
>>          2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>       
>>       On the other hand, the following statements are classified as safe:
>>       
>>          1 - INSERT INTO t_innodb SELECT * FROM t_myisam_temp
>>       
>>          2 - INSERT INTO t_myisam_temp SELECT * FROM t_innodb
>>       
>>       The patch also guarantees that transactions that have a DROP TEMPORARY are
>>       always written to the binary log regardless of the mode and the outcome:
>>       commit or rollback. In particular, the DROP TEMPORARY is extended with the
>>       IF EXISTS clause when the current statement logging format is set to row.
>>       
>>       Finally, the patch allows to switch from STATEMENT to MIXED/ROW when there
>>       are temporary tables but the contrary is not possible.
>>      @ mysql-test/extra/rpl_tests/rpl_implicit_commit_binlog.test
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/extra/rpl_tests/rpl_innodb.test
>>         Added comments to the test case based on the fact that in ROW mode
>>         DROP TEMPORARY is not written to the binary log.
> 
> The message is not correct according to the new patch
> 
>>      @ mysql-test/extra/rpl_tests/rpl_loaddata.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/include/ctype_utf8_table.inc
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/r/ctype_cp932_binlog_stm.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/binlog/r/binlog_database.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/binlog/r/binlog_row_binlog.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/binlog/r/binlog_row_drop_tmp_tbl.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/binlog/r/binlog_row_mix_innodb_myisam.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/binlog/t/binlog_tmp_table.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/suite/rpl/r/rpl_mixed_implicit_commit_binlog.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl/r/rpl_mixed_mixing_engines.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>         
>>         ******
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
> 
> 
> Please remove the duplicated message.
> 
>>      @ mysql-test/suite/rpl/r/rpl_mixed_row_innodb.result
>>         Removed the test case as it did not make sense in ROW mode.
>>      @ mysql-test/suite/rpl/r/rpl_row_drop.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl/r/rpl_row_implicit_commit_binlog.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl/r/rpl_row_mixing_engines.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl/r/rpl_stm_implicit_commit_binlog.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl/r/rpl_stm_innodb.result
>>         Added comments to the test case based on the fact that in ROW mode
>>         DROP TEMPORARY is not written to the binary log.
> 
> Please fix the message
> 
>>      @ mysql-test/suite/rpl/r/rpl_stm_mixing_engines.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl/r/rpl_temp_temporary.result
>>         Added a test case.
>>      @ mysql-test/suite/rpl/t/rpl000013.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/suite/rpl/t/rpl_mixed_row_innodb.test
>>         Removed the test case as it did not make sense in ROW mode.
>>      @ mysql-test/suite/rpl/t/rpl_temp_table.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/suite/rpl/t/rpl_temp_temporary.test
>>         Added a test case.
>>      @ mysql-test/suite/rpl/t/rpl_temporary.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/suite/rpl_ndb/r/rpl_ndb_row_implicit_commit_binlog.result
>>         Updated the test case due to the new rules: changes to
>>         temporary tables are written to the binary log in the
>>         boundaries of a transaction if there is any.
>>      @ mysql-test/suite/rpl_ndb/r/rpl_truncate_7ndb.result
>>         Updated the test case to remove references to positions
>>         in the binary log.
>>      @ mysql-test/suite/rpl_ndb/t/rpl_truncate_7ndb.test
>>         Updated the test case to remove references to positions
>>         in the binary log.
>>      @ mysql-test/t/ctype_cp932_binlog_stm.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ mysql-test/t/mysqlbinlog.test
>>         Suppressed warning messages due to the following cases:
>>         
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>      @ sql/log.cc
>>         Improved the code by creating several functions to hide decision
>>         on type of engine changed, commit/abort, etc:  
>>         
>>         . stmt_has_updated_non_trans_table
>>         
>>         . trans_has_updated_non_trans_table
>>         
>>         . ending_trans
>>         
>>         Updated the binlog_rollback function and the use of the 
>>         OPTION_KEEP_LOG which indincates when a temporary table was
>>         either created or dropped and as such the command must be 
>>         logged if not in MIXED mode and even while rolling back the
>>         transaction.
>>      @ sql/log.h
>>         Improved the code by creating several functions to hide decision
>>         on type of engine changed, commit/abort, etc.
>>      @ sql/log_event.cc
>>         Removed the setting of the OPTION_KEEP_LOG as it is related to CREATE
>>         TEMPORARY and DROP TEMPORARY and not to the type of engine (i.e.
>>         transactional or non-transactional).
>>      @ sql/log_event_old.cc
>>         Removed the setting of the OPTION_KEEP_LOG as it is related to CREATE
>>         TEMPORARY and DROP TEMPORARY and not to the type of engine (i.e.
>>         transactional or non-transactional).
>>      @ sql/sql_class.cc
>>         Classifies the following statements as unsafe:
>>            1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>>         
>>         On the other hand, the following statements are classified as safe:
>>         
>>            1 - INSERT INTO t_innodb SELECT * FROM t_myisam_temp
>>         
>>            2 - INSERT INTO t_myisam_temp SELECT * FROM t_innodb
>>      @ sql/sql_class.h
>>         It allows to switch from STATEMENT to MIXED/ROW when there are temporary
>>         tables but the contrary is not possible.
>>      @ sql/sql_table.cc
>>         Fixed the case that a DROP/DROP TEMPORARY that affects a temporary table
> in MIXED
>>         mode is written as a DROP TEMPORARY TABLE IF EXISTS because the table may
> not exist
>>         in the slave and due to the IF EXISTS token an error will never happen
> while
>>         processing the statement in the slave.
>>         
>>         Removed a function that was not being used.
>>
>>     removed:
>>       mysql-test/suite/rpl/r/rpl_mixed_row_innodb.result
>>       mysql-test/suite/rpl/t/rpl_mixed_row_innodb.test
>>     added:
>>       mysql-test/suite/rpl/r/rpl_temp_temporary.result
>>       mysql-test/suite/rpl/t/rpl_temp_temporary.test
>>     modified:
>>       mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test
>>       mysql-test/extra/rpl_tests/rpl_implicit_commit_binlog.test
>>       mysql-test/extra/rpl_tests/rpl_innodb.test
>>       mysql-test/extra/rpl_tests/rpl_loaddata.test
>>       mysql-test/include/ctype_utf8_table.inc
>>       mysql-test/r/ctype_cp932_binlog_stm.result
>>       mysql-test/suite/binlog/r/binlog_database.result
>>       mysql-test/suite/binlog/r/binlog_row_binlog.result
>>       mysql-test/suite/binlog/r/binlog_row_drop_tmp_tbl.result
>>       mysql-test/suite/binlog/r/binlog_row_mix_innodb_myisam.result
>>       mysql-test/suite/binlog/r/binlog_stm_binlog.result
>>       mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result
>>       mysql-test/suite/binlog/t/binlog_tmp_table.test
>>       mysql-test/suite/rpl/r/rpl_mixed_binlog_max_cache_size.result
>>       mysql-test/suite/rpl/r/rpl_mixed_implicit_commit_binlog.result
>>       mysql-test/suite/rpl/r/rpl_mixed_mixing_engines.result
>>       mysql-test/suite/rpl/r/rpl_row_drop.result
>>       mysql-test/suite/rpl/r/rpl_row_implicit_commit_binlog.result
>>       mysql-test/suite/rpl/r/rpl_row_mixing_engines.result
>>       mysql-test/suite/rpl/r/rpl_stm_binlog_max_cache_size.result
>>       mysql-test/suite/rpl/r/rpl_stm_implicit_commit_binlog.result
>>       mysql-test/suite/rpl/r/rpl_stm_innodb.result
>>       mysql-test/suite/rpl/r/rpl_stm_mixing_engines.result
>>       mysql-test/suite/rpl/t/rpl000013.test
>>       mysql-test/suite/rpl/t/rpl_misc_functions.test
>>       mysql-test/suite/rpl/t/rpl_temp_table.test
>>       mysql-test/suite/rpl/t/rpl_temporary.test
>>       mysql-test/suite/rpl_ndb/r/rpl_ndb_row_implicit_commit_binlog.result
>>       mysql-test/suite/rpl_ndb/r/rpl_truncate_7ndb.result
>>       mysql-test/suite/rpl_ndb/t/rpl_truncate_7ndb.test
>>       mysql-test/t/create_select_tmp.test
>>       mysql-test/t/ctype_cp932_binlog_stm.test
>>       mysql-test/t/mysqlbinlog.test
>>       sql/log.cc
>>       sql/log.h
>>       sql/log_event.cc
>>       sql/log_event_old.cc
>>       sql/sql_class.cc
>>       sql/sql_class.h
>>       sql/sql_table.cc
>> === modified file 'mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test'
>> --- a/mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test	2010-02-14
> 00:59:39 +0000
>> +++ b/mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test	2010-04-14
> 11:55:46 +0000
>> @@ -142,13 +142,13 @@ BEGIN;
>>  --eval INSERT INTO t1 (a, data) VALUES (21, 's');
>>  --enable_query_log
>>  
>> -if (`SELECT @@binlog_format = 'STATEMENT'`)
>> +if (`SELECT @@binlog_format = 'STATEMENT' || @@binlog_format = 'MIXED'`)
>>  {
>>    --disable_query_log
>>    CREATE TABLE t4 SELECT * FROM t1;
>>    --enable_query_log
>>  }
>> -if (`SELECT @@binlog_format = 'ROW' || @@binlog_format = 'MIXED'`)
>> +if (`SELECT @@binlog_format = 'ROW'`)
>>  {
>>    --disable_query_log
>>    --error ER_TRANS_CACHE_FULL, ER_ERROR_ON_WRITE
>> @@ -178,7 +178,7 @@ BEGIN;
>>  CREATE TABLE t5 (a int);
>>  --enable_query_log
>>  
>> -if (`SELECT @@binlog_format = 'ROW' || @@binlog_format = 'MIXED'` )
>> +if (`SELECT @@binlog_format = 'ROW'`)
>>  {
>>    connection slave;
>>    --source include/wait_for_slave_sql_to_stop.inc
>>
>> === modified file 'mysql-test/extra/rpl_tests/rpl_implicit_commit_binlog.test'
>> --- a/mysql-test/extra/rpl_tests/rpl_implicit_commit_binlog.test	2010-02-14
> 00:59:39 +0000
>> +++ b/mysql-test/extra/rpl_tests/rpl_implicit_commit_binlog.test	2010-04-14
> 11:55:46 +0000
>> @@ -362,10 +362,8 @@ while (`SELECT $ddl_cases >= 1`)
>>      #
>>      # 1: BEGIN
>>      # 2: CREATE TEMPORARY
>> -    # 3: COMMIT
>> -    # 4: BEGIN
>> -    # 5: INSERT
>> -    # 6: COMMIT
>> +    # 3: INSERT
>> +    # 4: COMMIT
>>      #
>>      # In RBR the transaction is not committed either and the statement is not
>>      # written to the binary log:
>> @@ -377,7 +375,7 @@ while (`SELECT $ddl_cases >= 1`)
>>      #
>>      if (`select @@binlog_format = 'STATEMENT' || @@binlog_format = 'MIXED'` )
>>      {
>> -      let $commit_event_row_number= 6;
>> +      let $commit_event_row_number= 4;
>>      }
>>      #
>>      # In NDB (RBR mode), the commit event is the sixth event
>> @@ -488,11 +486,9 @@ while (`SELECT $ddl_cases >= 1`)
>>      # In SBR, we have what follows:
>>      #
>>      # 1: BEGIN
>> -    # 2: DROP TEMPORARY
>> -    # 3: COMMIT
>> -    # 4: BEGIN
>> -    # 5: INSERT
>> -    # 6: COMMIT
>> +    # 2: INSERT
>> +    # 3: DROP TEMPORARY
>> +    # 4: COMMIT
>>      #
>>      # In RBR the transaction is not committed either and the statement is not
>>      # written to the binary log:
>> @@ -504,10 +500,6 @@ while (`SELECT $ddl_cases >= 1`)
>>      #
>>      if (`select @@binlog_format = 'STATEMENT'`)
>>      {
>> -      let $commit_event_row_number= 6;
>> -    }
>> -    if (`select @@binlog_format = 'ROW'`)
>> -    {
>>        let $commit_event_row_number= 4;
>>      }
>>      # In MIXED mode, the changes are logged as rows and we have what follows:
>> @@ -518,7 +510,7 @@ while (`SELECT $ddl_cases >= 1`)
>>      # 4: DROP TEMPORARY table IF EXISTS
>>      # 5: COMMIT
>>      #
>> -    if (`select @@binlog_format = 'MIXED'`)
>> +    if (`select @@binlog_format = 'MIXED' || @@binlog_format = 'ROW'`)
>>      {
>>        let $commit_event_row_number= 5;
>>      }
>> @@ -527,15 +519,18 @@ while (`SELECT $ddl_cases >= 1`)
>>      # in the binary log:
>>      #
>>      # 1: BEGIN
>> -    # 2: TABLE MAP EVENT
>> -    # 3: TABLE MAP EVENT (ndb_apply_status)
>> -    # 4: ROW EVENT
>> -    # 5: ROW EVENT
>> -    # 6: COMMIT
>> +    # 2: DROP TEMPORARY table IF EXISTS
>> +    # 3: COMMIT
>> +    # 4: BEGIN
>> +    # 5: TABLE MAP EVENT
>> +    # 6: TABLE MAP EVENT (ndb_apply_status)
>> +    # 7: ROW EVENT
>> +    # 8: ROW EVENT
>> +    # 9: COMMIT
>>      #
>>      if (`SELECT '$engine' = 'NDB'`)
>>      {
>> -      let $commit_event_row_number= 6;
>> +      let $commit_event_row_number= 9;
>>      }
>>      #
>>      # In NDB (MIXED mode), the commit event is the nineth event
>>
>> === modified file 'mysql-test/extra/rpl_tests/rpl_innodb.test'
>> --- a/mysql-test/extra/rpl_tests/rpl_innodb.test	2010-02-04 22:08:08 +0000
>> +++ b/mysql-test/extra/rpl_tests/rpl_innodb.test	2010-04-14 11:55:46 +0000
>> @@ -79,9 +79,9 @@ connection slave;
>>  # slave, the change will not be rolled back, so the inserted rows will
>>  # stay in t1 and we can verify that the transaction was replicated.
>>  #
>> -# Note, however, that the previous explanation is not true for ROW and
>> -# MIXED modes as rollback on a transactional table is not written to
>> -# the binary log.
>> +# Note, however, that the previous explanation is not true for ROW
>> +# as temporary tables are not written to the binary log and as such
>> +# a rollback in this case only truncates the internal cache.
> 
> Note should be removed.

ok. removed.

> 
> [snip]
> 
>> === added file 'mysql-test/suite/rpl/t/rpl_temp_temporary.test'
>> --- a/mysql-test/suite/rpl/t/rpl_temp_temporary.test	1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/suite/rpl/t/rpl_temp_temporary.test	2010-04-14 11:55:46 +0000
>> @@ -0,0 +1,166 @@
>>
> +################################################################################
>> +# BUG#51894 Replication failure with SBR on DROP TEMPORARY TABLE inside a
>> +#           transaction
>> +# BUG#52616 Temp table prevents switch binlog format from STATEMENT to ROW
>> +#
>> +# This test verifies what follows:
>> +#
>> +# 1 - If the following statements are classified as unsafe:
>> +#   1.1 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>> +#   1.2 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>> +#
>> +# 2 - If the following statements are classified as safe:
>> +#   1.7 - INSERT INTO t_innodb SELECT * FROM t_myisam_temp
>> +#   1.8 - INSERT INTO t_myisam_temp SELECT * FROM t_innodb
>> +#
>> +# 3 - If the following statements are classified as safe:
>> +#
> 
> 3 should be to verify if we can switch from statement to row format

ok. fixed.

> 
>> +# 4 - If statements that can manipulate a temporary table and do not cause an
>> +# implicit commit are kept in the boundaries of an on-going transaction. For
>> +# instance:
>> +#   4.1 - CREATE TEMPORARY TABLE
>> +#   4.2 - CREATE TEMPORARY TABLE LIKE
>> +#   4.3 - CREATE TEMPORARY TABLE SELECT * FROM
>> +#   4.4 - INSERT/UPDATE/DELETE (Note that only inserts are verified)
>> +#   4.5 - INSERT INTO t_myisam SELECT * FROM t_myisam_temp
>> +#   4.6 - INSERT INTO t_myisam_temp SELECT * FROM t_myisam
>> +#   4.7 - INSERT INTO t_innodb SELECT * FROM t_myisam_temp
>> +#   4.8 - INSERT INTO t_myisam_temp SELECT * FROM t_innodb
>> +#
>> +# 5 - If transactions that have a DROP TEMPORARY are always written to the 
>> +# binary log regardless of the mode and the outcome: commit or rollback.
>> +#
>> +# 6 - In particular, if the current statement logging format is set to row
>> +# the CREATE TEMPORARY is not logged and the DROP TEMPORARY is extended with
>> +# the IF EXISTS clause.
>> +#
>>
> +################################################################################
>> +
>> +source include/master-slave.inc;
>> +source include/have_binlog_format_statement.inc;
>> +source include/have_innodb.inc;
>> +
>> +CALL mtr.add_suppression("Unsafe statement binlogged in statement format since
> BINLOG_FORMAT = STATEMENT");
>> +
>> +--echo ########################################################################
>> +--echo #                         VERIFY ITEMS 1 and 2
>> +--echo ########################################################################
>> +
>> +--echo #
>> +--echo # Create temporary tables to verify when statements involving temporary
>> +--echo # tables are classified as safe or unsafe by printing out the warning
>> +--echo # messages.
>> +--echo #
>> +connection master;
>> +CREATE TABLE t_myisam(id int) engine= MyIsam;
>> +INSERT INTO t_myisam VALUES(1);
>> +CREATE TABLE t_innodb(id int) engine= Innodb;
>> +INSERT INTO t_innodb VALUES(1);
>> +CREATE TEMPORARY TABLE t_myisam_temp(id int) engine= MyIsam;
>> +INSERT INTO t_myisam_temp VALUES(1);
>> +CREATE TEMPORARY TABLE t_innodb_temp(id int) engine= Innodb;
>> +INSERT INTO t_innodb_temp VALUES(1);
>> +
>> +let $binlog_start= query_get_value("SHOW MASTER STATUS", Position, 1);
>> +BEGIN;
>> +INSERT INTO t_myisam SELECT * FROM t_myisam_temp;
>> +INSERT INTO t_innodb SELECT * FROM t_myisam_temp;
>> +INSERT INTO t_innodb SELECT * FROM t_innodb_temp;
>> +ROLLBACK;
>> +
>> +BEGIN;
>> +INSERT INTO t_myisam SELECT * FROM t_innodb_temp;
>> +INSERT INTO t_innodb SELECT * FROM t_myisam_temp;
>> +INSERT INTO t_innodb SELECT * FROM t_innodb_temp;
>> +ROLLBACK;
>> +
>> +--echo ########################################################################
>> +--echo #                          VERIFY ITEM 3
>> +--echo ########################################################################
>> +
>> +--echo #
>> +--echo # When there is a temporary table we can switch between the mixed and
>> +--echo # row formats. However, we cannot swith to the statement format.
> 
> typo: swith -> switch
> 
> 

ok. fixed.

> [snip]
> 
>> === modified file 'sql/log_event.cc'
>> --- a/sql/log_event.cc	2010-04-01 19:34:09 +0000
>> +++ b/sql/log_event.cc	2010-04-14 11:55:46 +0000
>> @@ -678,7 +678,7 @@ Log_event::Log_event(THD* thd_arg, uint1
>>  {
>>    server_id=	thd->server_id;
>>    when=		thd->start_time;
>> -  cache_type= (using_trans || stmt_has_updated_trans_table(thd)
>> +  cache_type= (using_trans || thd->thread_specific_used
>>                 ? Log_event::EVENT_TRANSACTIONAL_CACHE :
>>                 Log_event::EVENT_STMT_CACHE);
> 
> As agreed on irc, please fix this in the other bug.

ok. changed.

> 
>>  }
>> @@ -2470,6 +2470,7 @@ Query_log_event::Query_log_event(THD* th
>>    */
>>    LEX *lex= thd->lex;
>>    bool implicit_commit= FALSE;
>> +  bool force_trans= FALSE;
>>    cache_type= Log_event::EVENT_INVALID_CACHE;
>>    switch (lex->sql_command)
>>    {
>> @@ -2483,14 +2484,16 @@ Query_log_event::Query_log_event(THD* th
>>        implicit_commit= TRUE;
>>        break;
>>      case SQLCOM_DROP_TABLE:
>> -      implicit_commit= !(lex->drop_temporary &&
> thd->in_multi_stmt_transaction());
>> +      force_trans= lex->drop_temporary &&
> thd->in_multi_stmt_transaction();
>> +      implicit_commit= !force_trans;
>>        break;
>>      case SQLCOM_ALTER_TABLE:
>>      case SQLCOM_CREATE_TABLE:
>> -      implicit_commit= !((lex->create_info.options &
> HA_LEX_CREATE_TMP_TABLE) &&
>> -                   thd->in_multi_stmt_transaction()) &&
>> -                  !(lex->select_lex.item_list.elements &&
>> -                   thd->is_current_stmt_binlog_format_row());
>> +      force_trans= (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE)
> &&
>> +                    thd->in_multi_stmt_transaction();
>> +      implicit_commit= !force_trans &&
>> +                       !(lex->select_lex.item_list.elements &&
>> +                         thd->is_current_stmt_binlog_format_row());
>>        break;
>>      case SQLCOM_SET_OPTION:
>>        implicit_commit= (lex->autocommit ? TRUE : FALSE);
>> @@ -2548,7 +2551,8 @@ Query_log_event::Query_log_event(THD* th
>>    }
>>    else
>>    {
>> -    cache_type= (using_trans || stmt_has_updated_trans_table(thd)
>> +    cache_type= ((using_trans || stmt_has_updated_trans_table(thd) ||
>> +                 force_trans || thd->thread_specific_used)
>>                   ? Log_event::EVENT_TRANSACTIONAL_CACHE :
>>                   Log_event::EVENT_STMT_CACHE);
>>    }
>> @@ -7733,12 +7737,6 @@ int Rows_log_event::do_apply_event(Relay
>>        clear_all_errors(thd, const_cast<Relay_log_info*>(rli));
>>        error= 0;
>>      }
>> -
>> -    if (!use_trans_cache())
>> -    {
>> -      DBUG_PRINT("info", ("Marked that we need to keep log"));
>> -      thd->variables.option_bits|= OPTION_KEEP_LOG;
>> -    }
>>    } // if (table)
>>  
>>    
>>
>> === modified file 'sql/log_event_old.cc'
>> --- a/sql/log_event_old.cc	2010-04-01 19:34:09 +0000
>> +++ b/sql/log_event_old.cc	2010-04-14 11:55:46 +0000
>> @@ -241,11 +241,6 @@ Old_rows_log_event::do_apply_event(Old_r
>>      DBUG_EXECUTE_IF("stop_slave_middle_group",
>>                      const_cast<Relay_log_info*>(rli)->abort_slave=
> 1;);
>>      error= do_after_row_operations(table, error);
>> -    if (!ev->use_trans_cache())
>> -    {
>> -      DBUG_PRINT("info", ("Marked that we need to keep log"));
>> -      ev_thd->variables.option_bits|= OPTION_KEEP_LOG;
>> -    }
>>    }
>>  
>>    if (error)
>> @@ -1683,11 +1678,6 @@ int Old_rows_log_event::do_apply_event(R
>>      DBUG_EXECUTE_IF("stop_slave_middle_group",
>>                      const_cast<Relay_log_info*>(rli)->abort_slave=
> 1;);
>>      error= do_after_row_operations(rli, error);
>> -    if (!use_trans_cache())
>> -    {
>> -      DBUG_PRINT("info", ("Marked that we need to keep log"));
>> -      thd->variables.option_bits|= OPTION_KEEP_LOG;
>> -    }
>>    } // if (table)
>>  
>>    if (error)
>>
>> === modified file 'sql/sql_class.cc'
>> --- a/sql/sql_class.cc	2010-04-07 12:02:19 +0000
>> +++ b/sql/sql_class.cc	2010-04-14 11:55:46 +0000
>> @@ -3641,15 +3641,76 @@ int THD::decide_logging_format(TABLE_LIS
>>          if (prev_write_table && prev_write_table->file->ht !=
>>              table->table->file->ht)
>>            multi_engine= TRUE;
>> +        /*
>> +          Every temporary table must be always written to the binary
>> +          log in transaction boundaries and as such we artificially
>> +          classify them as transactional.
>> +          
>> +          Indirectly, this avoids classifying a temporary table created
>> +          on a non-transactional engine as unsafe when it is modified
>> +          after any transactional table:
>> +
>> +          BEGIN;
>> +            INSERT INTO innodb_t VALUES (1);
>> +            INSERT INTO myisam_t_temp VALUES (1);
>> +          COMMIT;
>> +
>> +          BINARY LOG:
>> +
>> +          BEGIN;
>> +            INSERT INTO innodb_t VALUES (1);
>> +            INSERT INTO myisam_t_temp VALUES (1);
>> +          COMMIT;
>> +        */
>>          all_trans_engines= all_trans_engines &&
>> -                           table->table->file->has_transactions();
>> +                           (table->table->file->has_transactions() ||
>> +                            table->table->s->tmp_table);
>>          prev_write_table= table->table;
>>          flags_all_set &= flags;
>>          flags_some_set |= flags;
>>        }
>> -      if (prev_access_table && prev_access_table->file->ht !=
> table->table->file->ht)
>> -        mixed_engine= mixed_engine ||
> (prev_access_table->file->has_transactions() !=
>> -                      table->table->file->has_transactions());
>> +      /*
>> +        The mixture of non-transactional and transactional tables must
>> +        indentified and classified as unsafe. However, a temporary table
> 
> typo: identified

ok. fixed.
> 
> [snip]
> 
> 
Thread
bzr commit into mysql-next-mr-bugfixing branch (alfranio.correia:3155)Bug#51894 Bug#52616Alfranio Correia14 Apr
  • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616He Zhenxing15 Apr
    • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616Alfranio Correia15 Apr
      • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616He Zhenxing16 Apr
        • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616Alfranio Correia16 Apr
          • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616Alfranio Correia19 Apr
            • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616He Zhenxing20 Apr
              • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616Alfranio Correia20 Apr
                • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616He Zhenxing20 Apr
                  • Re: bzr commit into mysql-next-mr-bugfixing branch (alfranio.correia:3155) Bug#51894 Bug#52616Paul DuBois20 Apr
                    • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616Alfranio Correia20 Apr
                      • Re: bzr commit into mysql-next-mr-bugfixing branch (alfranio.correia:3155) Bug#51894 Bug#52616Paul DuBois20 Apr
                        • Re: bzr commit into mysql-next-mr-bugfixing branch(alfranio.correia:3155) Bug#51894 Bug#52616Alfranio Correia20 Apr
                          • Re: bzr commit into mysql-next-mr-bugfixing branch (alfranio.correia:3155) Bug#51894 Bug#52616Paul DuBois20 Apr