Hi Libing,
Nice work, patch approved after addressing the following comments.
Li-Bing.Song@stripped wrote:
> #At file:///home/anders/work/bzrwork1/wt1/mysql-next-mr-bugfixing/ based on
> revid:tor.didriksen@stripped
>
> 3257 Li-Bing.Song@stripped 2010-09-03
> 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 another
> non-transactional table is modified. On RBR, only the row events of
> the other non-transactional table are binlogged.
> @ mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test
> After this patch, It will not generate an incident event, even the
> creating table is a MyIASM table. As 'CREATE TABLE ...' part and row
> events of the creating table will never be binlogged and slave will
> always keep in consisteny with master.
> @ mysql-test/include/rpl_diff_tables.inc
> Always switches to master at the end.
> @ mysql-test/suite/rpl/r/rpl_row_binlog_max_cache_size.result
> After this patch, It will not generate an incident event, even the
> creating table is a MyIASM table. As 'CREATE TABLE ...' part and row
> events of the creating table will never be binlogged and slave will
> always keep in consisteny with master.
> @ mysql-test/suite/rpl/t/rpl_create_table.result
> Rename this file. Now not only 'CREATE TABLE IF NOT EXISTS' but also
> all other 'CREATE TABLE' statemens' tests can be put in this file.
>
> Added test for bug#39804.
> @ mysql-test/suite/rpl/t/rpl_create_table.test
> Rename this file. Now not only 'CREATE TABLE IF NOT EXISTS' but also
> all other 'CREATE TABLE' statemens' tests can be put in this file.
>
statements
> Added test for bug#39804.
> @ sql/binlog.cc
> Trx-cache is always able to be truncated when a SINGLE statement transaction
> rolls back and is on RBR.
> @ 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.
>
transactional
> tmp_disable_binlog() appeared to be redundant in the context of
> create_select::abort_result_set(). It's correct to calculate value of
> thd->transaction.stmt.modified_non_trans_table before
> create_select::abort_result_set() that logs in STMT format a way
> dependent on `modified_non_trans_table'.
> The calculated value continues to control logging in ROW format same way
> as it was in the base code before the patch.
> @ sql/sql_parse.cc
> OPTION_KEEP_LOG should be set only when the temporary table is
> created successfully on RBR.
>
> renamed:
> mysql-test/suite/rpl/r/rpl_create_if_not_exists.result =>
> mysql-test/suite/rpl/t/rpl_create_table.result
> mysql-test/suite/rpl/t/rpl_create_if_not_exists.test =>
> mysql-test/suite/rpl/t/rpl_create_table.test
> modified:
> mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test
> mysql-test/include/rpl_diff_tables.inc
> mysql-test/suite/rpl/r/rpl_row_binlog_max_cache_size.result
> sql/binlog.cc
> sql/sql_class.h
> sql/sql_insert.cc
> sql/sql_parse.cc
> mysql-test/suite/rpl/t/rpl_create_table.result
> mysql-test/suite/rpl/t/rpl_create_table.test
> === 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-06-30 20:56:21
> +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_binlog_max_cache_size.test 2010-09-03 04:10:22
> +0000
> @@ -177,17 +177,9 @@ BEGIN;
> --disable_query_log
> CREATE TABLE t5 (a int);
> --enable_query_log
> +sync_slave_with_master;
>
> -if (`SELECT @@binlog_format = 'ROW'`)
> -{
> - connection slave;
> - --source include/wait_for_slave_sql_to_stop.inc
> -
> - SET GLOBAL SQL_SLAVE_SKIP_COUNTER = 1;
> - START SLAVE SQL_THREAD;
> - --source include/wait_for_slave_sql_to_start.inc
> - connection master;
> -}
> +connection master;
>
> let $diff_statement= SELECT * FROM t1;
> --source include/diff_master_slave.inc
>
> === modified file 'mysql-test/include/rpl_diff_tables.inc'
> --- a/mysql-test/include/rpl_diff_tables.inc 2010-07-04 04:02:49 +0000
> +++ b/mysql-test/include/rpl_diff_tables.inc 2010-09-03 04:10:22 +0000
> @@ -33,3 +33,4 @@ while (`SELECT "XX$_servers" <> "XX"`)
> --source include/diff_tables.inc
> connection $_slave;
> }
> +connection $_master;
>
> === modified file 'mysql-test/suite/rpl/r/rpl_row_binlog_max_cache_size.result'
> --- a/mysql-test/suite/rpl/r/rpl_row_binlog_max_cache_size.result 2010-04-28 12:47:49
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_row_binlog_max_cache_size.result 2010-09-03 04:10:22
> +0000
> @@ -39,8 +39,6 @@ Got one of the listed errors
> BEGIN;
> Got one of the listed errors
> Got one of the listed errors
> -SET GLOBAL SQL_SLAVE_SKIP_COUNTER = 1;
> -START SLAVE SQL_THREAD;
> source include/diff_master_slave.inc;
>
> ########################################################################################
> # 3 - BEGIN - COMMIT
>
> === renamed file 'mysql-test/suite/rpl/r/rpl_create_if_not_exists.result' =>
> 'mysql-test/suite/rpl/t/rpl_create_table.result'
> --- a/mysql-test/suite/rpl/r/rpl_create_if_not_exists.result 2010-08-18 09:35:41
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_create_table.result 2010-09-03 04:10:22 +0000
> @@ -126,3 +126,57 @@ show binlog events from <binlog_start>;
> Log_name Pos Event_type Server_id End_log_pos Info
> DROP VIEW v1;
> DROP TABLE t1, t2;
> +
> +# BUG#39804 Failing CREATE ... SELECT doesn't appear in binary log.
> +# -----------------------------------------------------------------
> +
> +DROP FUNCTION IF EXISTS f1;
> +CREATE TABLE t2(a INT) ENGINE=MyISAM;
> +CREATE TABLE t3(a INT KEY) ENGINE=Innodb;
> +CREATE FUNCTION f_insert_t2() RETURNS INT
> +BEGIN
> +INSERT INTO t2 VALUES(1);
> +RETURN 1;
> +END|
> +CREATE FUNCTION f_insert_t3() RETURNS INT
> +BEGIN
> +INSERT INTO t3 VALUES(1);
> +RETURN 1;
> +END|
> +
> +# Create a Innodb table and no any other table is modified
> +CREATE TABLE t1(UNIQUE(a)) ENGINE=Innodb
> +SELECT 1 AS a UNION ALL SELECT 1;
> +ERROR 23000: Duplicate entry '1' for key 'a'
> +# Nothing should be binlogged
> +show binlog events in 'master-bin.000001' from <binlog_start>;
> +Log_name Pos Event_type Server_id End_log_pos Info
> +
> +# Create a Innodb table and both of the transaction table(t3) and
> +# non-transaction table(t2) are updated.
> +CREATE TABLE t1(UNIQUE(a)) ENGINE=Innodb
> +SELECT 1 AS a UNION ALL SELECT f_insert_t3 ();
> +ERROR 23000: Duplicate entry '1' for key 'a'
> +# Nothing should be binlogged
> +show binlog events in 'master-bin.000001' from <binlog_start>;
> +Log_name Pos Event_type Server_id End_log_pos Info
> +
> +# Create a Innodb table and both of the transaction table(t3) and
> +# non-transaction table(t2) are updated.
> +CREATE TABLE t1(UNIQUE(a)) ENGINE=Innodb
> +SELECT f_insert_t2() AS a UNION ALL SELECT f_insert_t3 ();
> +ERROR 23000: Duplicate entry '1' for key 'a'
> +Comparing tables master:test.t2 and slave:test.t2
> +Comparing tables master:test.t3 and slave:test.t3
> +
> +# Create a Innodb table and non-transaction table(t2) is updated and
> +# inserting transaction table(t3) cause an error(duplicate key).
> +INSERT INTO t3 VALUES(1);
> +CREATE TABLE t1 ENGINE=Innodb
> +SELECT f_insert_t2() AS a UNION ALL SELECT f_insert_t3 ();
> +ERROR 23000: Duplicate entry '1' for key 'PRIMARY'
> +Comparing tables master:test.t2 and slave:test.t2
> +Comparing tables master:test.t3 and slave:test.t3
> +DROP TABLE t2, t3;
> +DROP FUNCTION f_insert_t2;
> +DROP FUNCTION f_insert_t3;
>
> === renamed file 'mysql-test/suite/rpl/t/rpl_create_if_not_exists.test' =>
> 'mysql-test/suite/rpl/t/rpl_create_table.test'
> --- a/mysql-test/suite/rpl/t/rpl_create_if_not_exists.test 2010-08-18 09:35:41 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_create_table.test 2010-09-03 04:10:22 +0000
> @@ -28,6 +28,7 @@
> #
>
> source include/master-slave.inc;
> +source include/have_innodb.inc;
> disable_warnings;
> DROP DATABASE IF EXISTS mysqltest;
>
> @@ -173,4 +174,163 @@ DROP VIEW v1;
>
> DROP TABLE t1, t2;
>
> +--echo
> +--echo # BUG#39804 Failing CREATE ... SELECT doesn't appear in binary log.
> +--echo # -----------------------------------------------------------------
> +--echo
> +--disable_warnings
> +DROP FUNCTION IF EXISTS f1;
> +--enable_warnings
> +
> +CREATE TABLE t2(a INT) ENGINE=MyISAM;
> +CREATE TABLE t3(a INT KEY) ENGINE=Innodb;
> +
> +--delimiter |
> +CREATE FUNCTION f_insert_t2() RETURNS INT
> +BEGIN
> + INSERT INTO t2 VALUES(rand());
> + RETURN 1;
> +END|
> +
> +CREATE FUNCTION f_insert_t3() RETURNS INT
> +BEGIN
> + INSERT INTO t3 VALUES(1);
> + RETURN 1;
> +END|
> +--delimiter ;
> +
> +let $_is_RBR= `SELECT @@binlog_format = 'ROW'`;
> +let $_n= 2;
> +while ($_n)
> +{
> +
> + let $_engine= MyISAM;
> + if (`SELECT $_n = 2`)
> + {
> + let $_engine= Innodb;
> + }
> + dec $_n;
> +
> + let _m= 2;
> + while ($_m)
> + {
> + let $_temp_option=;
> + if (`SELECT $_m = 1`)
> + {
> + let $_temp_option= TEMPORARY;
> + }
> + dec $_m;
> + --let binlog_start= query_get_value(SHOW MASTER STATUS, Position, 1)
> + --let binlog_file= query_get_value(SHOW MASTER STATUS, File, 1)
> +
> + --echo
> + --echo # Create a $_engine table and no any other table is modified
> + --error ER_DUP_ENTRY
> + eval CREATE $_temp_option TABLE t1(UNIQUE(a)) ENGINE=$_engine
> + SELECT 1 AS a UNION ALL SELECT 1;
> + --echo # Nothing should be binlogged
> + source include/show_binlog_events.inc;
> +
> + --echo
> + --echo # Create a $_engine table and another transactional table(t3) is
> updated.
> + --error ER_DUP_ENTRY
> + eval CREATE $_temp_option TABLE t1(UNIQUE(a)) ENGINE=$_engine
> + SELECT 1 AS a UNION ALL SELECT f_insert_t3 ();
> + --echo # Nothing should be binlogged
> + source include/show_binlog_events.inc;
> +
> + --echo
> + --echo # Create a $_engine table and both of the transaction table(t3) and
> + --echo # non-transaction table(t2) are updated.
> +
> + --error ER_DUP_ENTRY
> + eval CREATE $_temp_option TABLE t1(UNIQUE(a)) ENGINE=$_engine
> + SELECT f_insert_t3() AS a UNION ALL SELECT f_insert_t2 ();
> + if ($_is_RBR)
> + {
> + # There should be only 4 events binlogged and 'CREATE TABLE' should not be
> + # binlogged
> + # BEGIN
> + # Table_map
> + # Row event
> + # COMMIT
> +
> + let $event= query_get_value(SHOW BINLOG EVENTS IN '$binlog_file' FROM
> $binlog_start, Info, 5);
> + if (`SELECT '$event' <> 'No such row'`)
> + {
> + show binlog events;
> + --echo Unexpected event is binlogged: '$event'.
> + --die Unexpected event is binlogged.
> + }
I'd suggest to also check that the first 4 events are actually BEGIN,
Table_map, Row_event and COMMIT.
> + }
> +
> + if (!_is_RBR)
> + {
> + # The CREATE TABLE ... SELECT should be binlogged.
> + let $event= query_get_value(SHOW BINLOG EVENTS IN '$binlog_file' FROM
> $binlog_start, Info, 1);
> + if (`SELECT '$event' == 'No such row'`)
> + {
> + --echo Expected event(CREATE TABLE ... SELECT) is not binlogged.
> + --die Expected event(CREATE TABLE ... SELECT) is not binlogged.
> + }
> + }
> + let $diff_table=test.t2;
> + source include/rpl_diff_tables.inc;
> + let $diff_table=test.t3;
> + source include/rpl_diff_tables.inc;
> +
> + --echo
> + --echo # Create a $_engine table and non-transaction table(t2) is updated and
> + --echo # inserting transaction table(t3) cause an error(duplicate key). It
> means
> + --echo # error happens before the table is created.
> + INSERT INTO t3 VALUES(1);
> +
> + --let binlog_start= query_get_value(SHOW MASTER STATUS, Position, 1)
> + --let binlog_file= query_get_value(SHOW MASTER STATUS, File, 1)
> +
> + --error ER_DUP_ENTRY
> + eval CREATE $_temp_option TABLE t1 ENGINE=$_engine
> + SELECT f_insert_t2() AS a UNION ALL SELECT f_insert_t3 ();
> + if ($_is_RBR)
> + {
> + # There should be only 4 events binlogged and 'CREATE TABLE' should not be
> + # binlogged
> + # BEGIN
> + # Table_map
> + # Row event
> + # COMMIT
> + let $event= query_get_value(SHOW BINLOG EVENTS IN '$binlog_file' FROM
> $binlog_start, Info, 5);
> + if (`SELECT '$event' <> 'No such row'`)
> + {
> + show binlog events;
> + --echo Unexpected event is binlogged: '$event'.
> + --die Unexpected event is binlogged.
> + }
> + }
> +
> + if (!_is_RBR)
> + {
> + # The CREATE TABLE ... SELECT should be binlogged.
> + let $event= query_get_value(SHOW BINLOG EVENTS IN '$binlog_file' FROM
> $binlog_start, Info, 1);
> + if (`SELECT '$event' == 'No such row'`)
> + {
> + --echo Expected event(CREATE TABLE ... SELECT) is not binlogged.
> + --die Expected event(CREATE TABLE ... SELECT) is not binlogged.
> + }
> + }
> +
> + let $diff_table=test.t2;
> + source include/rpl_diff_tables.inc;
> + let $diff_table=test.t3;
> + source include/rpl_diff_tables.inc;
> + TRUNCATE TABLE t2;
> + TRUNCATE TABLE t3;
> + }
> +}
> +
> +--echo
> +--echo # CREATE TEMPORARY TABLE
> +DROP TABLE t2, t3;
> +DROP FUNCTION f_insert_t2;
> +DROP FUNCTION f_insert_t3;
> source include/master-slave-end.inc;
>
> === modified file 'sql/binlog.cc'
> --- a/sql/binlog.cc 2010-08-20 03:37:42 +0000
> +++ b/sql/binlog.cc 2010-09-03 04:10:22 +0000
> @@ -639,7 +639,7 @@ static int binlog_rollback(handlerton *h
> thd->variables.binlog_format == BINLOG_FORMAT_MIXED) ||
> (trans_has_updated_non_trans_table(thd) &&
> ending_single_stmt_trans(thd,all) &&
> - thd->variables.binlog_format == BINLOG_FORMAT_MIXED)))
> + !thd->is_current_stmt_binlog_format_row())))
> {
> Query_log_event qev(thd, STRING_WITH_LEN("ROLLBACK"), TRUE, FALSE, TRUE, 0);
> error= binlog_flush_trx_cache(thd, cache_mngr, &qev);
>
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h 2010-08-20 09:15:16 +0000
> +++ b/sql/sql_class.h 2010-09-03 04:10:22 +0000
> @@ -3006,6 +3006,7 @@ class select_create: public select_inser
> MYSQL_LOCK *m_lock;
> /* m_lock or thd->extra_lock */
> MYSQL_LOCK **m_plock;
> + bool modified_non_trans_table_by_select;
> public:
> select_create (TABLE_LIST *table_arg,
> HA_CREATE_INFO *create_info_par,
> @@ -3017,7 +3018,8 @@ public:
> create_info(create_info_par),
> select_tables(select_tables_arg),
> alter_info(alter_info_arg),
> - m_plock(NULL)
> + m_plock(NULL),
> + modified_non_trans_table_by_select(FALSE)
> {}
> int prepare(List<Item> &list, SELECT_LEX_UNIT *u);
>
>
> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc 2010-08-24 14:23:04 +0000
> +++ b/sql/sql_insert.cc 2010-09-03 04:10:22 +0000
> @@ -3529,9 +3529,23 @@ void select_insert::abort_result_set() {
> query_cache_invalidate3(thd, table, 1);
> }
> DBUG_ASSERT(transactional_table || !changed ||
> - thd->transaction.stmt.modified_non_trans_table);
> + thd->transaction.stmt.modified_non_trans_table || can_rollback_data());
> table->file->ha_release_auto_increment();
> }
> + else if (thd->transaction.stmt.modified_non_trans_table &&
> + mysql_bin_log.is_open())
> + {
> + /*
> + It is a rare case. It happens only when SELECT clause calls a function
> + in which a non-transaction table is modified and it encounters an error
> + before the table is created.
> + */
> + int errcode= query_error_code(thd, thd->killed == THD::NOT_KILLED);
> + /* error of writing binary log is ignored */
> + (void) thd->binlog_query(THD::ROW_QUERY_TYPE, thd->query(),
> + thd->query_length(),
> + TRUE, FALSE, FALSE, errcode);
> + }
>
> DBUG_VOID_RETURN;
> }
> @@ -3800,6 +3814,13 @@ select_create::prepare(List<Item> &value
> thd->binlog_start_trans_and_stmt();
> }
>
> + /*
> + All operations of the store functions which are called in SELECT clause has
> + finished before calling prepare().
> + */
> + modified_non_trans_table_by_select=
> + thd->transaction.stmt.modified_non_trans_table;
> +
> DBUG_ASSERT(create_table->table == NULL);
>
> DBUG_EXECUTE_IF("sleep_create_select_before_check_if_exists",
> my_sleep(6000000););
> @@ -3983,7 +4004,17 @@ void select_create::abort_result_set()
> DBUG_ENTER("select_create::abort_result_set");
>
> /*
> - In select_insert::abort() we roll back the statement, including
> + 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 preceeding select in order to binlog
> + correctly. And it does so through recording the select's value in
> + select_create::prepare() to restore it here.
> + */
> + if (table)
> + thd->transaction.stmt.modified_non_trans_table=
> + modified_non_trans_table_by_select;
> + /*
> + 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.
> @@ -3997,10 +4028,8 @@ void select_create::abort_result_set()
> of the table succeeded or not, since we need to reset the binary
> log state.
> */
> - tmp_disable_binlog(thd);
> select_insert::abort_result_set();
> - thd->transaction.stmt.modified_non_trans_table= FALSE;
> - reenable_binlog(thd);
> +
> /* possible error of writing binary log is ignored deliberately */
> (void) thd->binlog_flush_pending_rows_event(TRUE, TRUE);
>
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2010-08-20 09:15:16 +0000
> +++ b/sql/sql_parse.cc 2010-09-03 04:10:22 +0000
> @@ -2537,10 +2537,6 @@ case SQLCOM_PREPARE:
> */
> lex->unlink_first_table(&link_to_local);
>
> - /* So that CREATE TEMPORARY TABLE gets to binlog at commit/rollback */
> - if (create_info.options & HA_LEX_CREATE_TMP_TABLE)
> - thd->variables.option_bits|= OPTION_KEEP_LOG;
> -
There is another place that also set OPTION_KEEP_LOG for CREATE
TEMPORARY TABLE without SELECT, which I think should also be removed.
> /*
> select_create is currently not re-execution friendly and
> needs to be created for every execution of a PS/SP.
> @@ -2585,6 +2581,11 @@ case SQLCOM_PREPARE:
> if (!res)
> my_ok(thd);
> }
> + /* So that CREATE TEMPORARY TABLE gets to binlog at commit/rollback */
> + if (!res && (create_info.options & HA_LEX_CREATE_TMP_TABLE)
> &&
> + thd->in_multi_stmt_transaction_mode() &&
> + !thd->is_current_stmt_binlog_format_row())
> + thd->variables.option_bits|= OPTION_KEEP_LOG;
>
> end_with_restore_list:
> break;
