Hi Libing,
Nice work!
STATUS
------
Not approved!
REQUIRED CHANGES
----------------
RC2.
+let $_n= 2;
+if ($_n)
+{
+ dec $_n;
+
+ let $_engine= MyISAM;
+ if ($_n)
+ {
+ let $_engine= Innodb;
+ }
I think the if ($_n) should while ($_n), so that both MyISAM and InnoDB
will be tested. And please use a more explicit way to do that, such as:
let $_n= 2;
while ($_n)
{
if (`SELECT $_n = 1`)
let $_engine= MyISAM;
if (`SELECT $_n = 2`)
let $_engine= InnoDB;
dec $_n;
...
}
RC2. There would be a crash on the following ASSERTION in
select_insert::abort_result_set() when testing with $_engine set to
MyISAM instead of InnoDB:
DBUG_ASSERT(transactional_table || !changed ||
thd->transaction.stmt.modified_non_trans_table);
RC3. I'd suggest rename:
modified_by_select_non_trans_table
to
modified_non_trans_table_by_select
Please also look for some inline comments.
Li-Bing.Song@stripped wrote:
> #At file:///home/anders/work/bzrwork1/wt1/mysql-next-mr-bugfixing/ based on
> revid:bernt.johnsen@stripped
>
> 3227 Li-Bing.Song@stripped 2010-08-26
> 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.
>
dropt => dropped
> 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.
>
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.
>
> 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/include/rpl_diff_tables.inc
> sql/sql_class.h
> sql/sql_insert.cc
> mysql-test/suite/rpl/t/rpl_create_table.result
> mysql-test/suite/rpl/t/rpl_create_table.test
> === 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-08-26 01:58:02 +0000
> @@ -33,3 +33,4 @@ while (`SELECT "XX$_servers" <> "XX"`)
> --source include/diff_tables.inc
> connection $_slave;
> }
> +connection $_master;
>
> === 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-08-26 01:58:02 +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-08-26 01:58:02 +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,147 @@ 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(1);
> + RETURN 1;
> +END|
> +
> +CREATE FUNCTION f_insert_t3() RETURNS INT
> +BEGIN
> + INSERT INTO t3 VALUES(1);
> + RETURN 1;
> +END|
> +--delimiter ;
> +
> +let $_n= 2;
> +if ($_n)
> +{
> + dec $_n;
> +
> + let $_engine= MyISAM;
> + if ($_n)
> + {
> + let $_engine= Innodb;
> + }
> +
> + --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 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 both of the transaction table(t3) and
> + --echo # non-transaction table(t2) are updated.
Only t3 is updated, please fix the above comment.
> + --error ER_DUP_ENTRY
> + eval CREATE 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 TABLE t1(UNIQUE(a)) ENGINE=$_engine
> + SELECT f_insert_t2() AS a UNION ALL SELECT f_insert_t3 ();
> + let $_is_RBR= `SELECT @@binlog_format = 'ROW'`;
> + 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'`)
> + {
> + --echo Unexpected event is binlogged: '$event'.
> + --die Unexpected event is binlogged.
> + }
> + }
Hmm, I think this should be improved, the above only checks that there
is no more than 4 events generated. But no guarantee there are four such
events.
> +
> + 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 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'`)
> + {
> + --echo Unexpected event is binlogged: '$event'.
> + --die Unexpected event is binlogged.
> + }
> + }
Same as above.
> +
> + 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.
> + }
> + }
> +
same here
> + let $diff_table=test.t2;
> + source include/rpl_diff_tables.inc;
> + let $diff_table=test.t3;
> + source include/rpl_diff_tables.inc;
> +}
> +
> +DROP TABLE t2, t3;
> +DROP FUNCTION f_insert_t2;
> +DROP FUNCTION f_insert_t3;
> source include/master-slave-end.inc;
>
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h 2010-08-18 08:14:49 +0000
> +++ b/sql/sql_class.h 2010-08-26 01:58:02 +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_by_select_non_trans_table;
> 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_by_select_non_trans_table(FALSE)
> {}
> int prepare(List<Item> &list, SELECT_LEX_UNIT *u);
>
>
> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc 2010-08-18 10:18:27 +0000
> +++ b/sql/sql_insert.cc 2010-08-26 01:58:02 +0000
> @@ -3457,6 +3457,21 @@ void select_insert::abort_result_set() {
> thd->transaction.stmt.modified_non_trans_table);
> 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(),
> + FALSE, FALSE, FALSE, errcode);
> + }
> +
>
> DBUG_VOID_RETURN;
> }
> @@ -3727,6 +3742,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_by_select_non_trans_table=
> + 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););
> @@ -3910,7 +3932,26 @@ 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 and can modify a
> + non-transactional table. The current statement has to inherit
> + `modified_non_trans_table` of preceeding select in order to binlog
preceding
> + 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=
> + (thd->is_current_stmt_binlog_format_row() ?
> + FALSE : modified_by_select_non_trans_table);
> +
> + /*
> + 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 +3965,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);
>
