List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:August 26 2010 6:05am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)
Bug#39804
View as plain text  
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);
>  



Thread
bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227) Bug#39804Li-Bing.Song26 Aug
  • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)Bug#39804He Zhenxing26 Aug
Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)Bug#39804Libing Song26 Aug
Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3227)Bug#39804Libing Song30 Aug