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


Thread
bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257) Bug#39804Li-Bing.Song3 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257)Bug#39804He Zhenxing6 Sep
    • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257)Bug#39804Libing Song6 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257)Bug#39804Konstantin Osipov16 Sep
    • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257)Bug#39804Libing Song17 Sep
      • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257)Bug#39804Konstantin Osipov17 Sep
        • Re: bzr commit into mysql-next-mr-bugfixing branch (Li-Bing.Song:3257)Bug#39804Libing Song17 Sep