MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:July 30 2009 12:44pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:2941)
Bug#45574
View as plain text  
Hi Libing,

Find some comments in-line.

Cheers.

Li-Bing.Song@stripped wrote:
> #At file:///home/anders/work/bzrwork/bug45574/mysql-5.1-bugteam/ based on
> revid:davi.arnaut@stripped
>
>  2941 Li-Bing.Song@stripped	2009-07-28
>       BUG#45574 CREATE IF NOT EXISTS is not binlogged if the object exists
>       
>       There is an inconsistency with DROP DATABASE|TABLE IF EXISTS and CREATE
>       EVENT IF NOT EXISTS:DROP IF EXISTS statements are binlogged even if the DB or
> TABLE does not
>       exist, CREATE EVENT IF NOT EXISTS are binlogged even if the event exists.
> whereas CREATE
>       DATEBASE|TABLE IF NOT EXIST dose not binlog when the DB or TABLE exists. 
>       
>       This patch includes 2 tests to test all CREATE IF NOT EXISTS statement.
>       CREATE DATABASE IF NOT EXISTS,
>       CREATE TABLE IF NOT EXISTS,
>       CREATE TABLE IF NOT EXISTS ... LIKE have this bug.
>       CREAET TABLE IF NOT EXISTS ... SELECT has this bug in row mode.
>       This patch adds code to fix all above.
>
>     added:
>       mysql-test/suite/rpl/r/rpl_create_if_not_exists.result
>       mysql-test/suite/rpl/r/rpl_create_tmp_table_if_not_exists.result
>       mysql-test/suite/rpl/t/rpl_create_if_not_exists.test
>       mysql-test/suite/rpl/t/rpl_create_tmp_table_if_not_exists.test
>     modified:
>       sql/sql_db.cc
>       sql/sql_insert.cc
>       sql/sql_table.cc
> === added file 'mysql-test/suite/rpl/r/rpl_create_if_not_exists.result'
> --- a/mysql-test/suite/rpl/r/rpl_create_if_not_exists.result	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_create_if_not_exists.result	2009-07-28 08:19:57
> +0000
> @@ -0,0 +1,33 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +DROP DATABASE IF EXISTS mysqltest;
> +CREATE DATABASE IF NOT EXISTS mysqltest;
> +USE mysqltest;
> +CREATE TABLE IF NOT EXISTS t(c1 int);
> +CREATE TABLE IF NOT EXISTS t1 LIKE t;
> +CREATE TABLE IF NOT EXISTS t2 SELECT * FROM t;
> +CREATE EVENT IF NOT EXISTS e 
> +ON SCHEDULE AT CURRENT_TIMESTAMP + INTERVAL 1 HOUR 
> +DO SELECT now();
> +DROP DATABASE mysqltest;
> +CREATE DATABASE IF NOT EXISTS mysqltest;
> +USE mysqltest;
> +CREATE TABLE IF NOT EXISTS t(c1 int);
> +CREATE TABLE IF NOT EXISTS t1 LIKE t;
> +CREATE TABLE IF NOT EXISTS t2 SELECT * FROM t;
> +CREATE EVENT IF NOT EXISTS e 
> +ON SCHEDULE AT CURRENT_TIMESTAMP + INTERVAL 1 HOUR 
> +DO SELECT now();
> +SHOW TABLES in mysqltest;
> +Tables_in_mysqltest
> +t
> +t1
> +t2
> +SHOW EVENTS in mysqltest;
> +Db	Name	Definer	Time zone	Type	Execute at	Interval value	Interval
> field	Starts	Ends	Status	Originator	character_set_client	collation_connection	Database
> Collation
> +mysqltest	e	@	SYSTEM	ONE
> TIME	-	NULL	NULL	NULL	NULL	SLAVESIDE_DISABLED	1	latin1	latin1_swedish_ci	latin1_swedish_ci
> +DROP DATABASE IF EXISTS mysqltest;
>
> === added file 'mysql-test/suite/rpl/r/rpl_create_tmp_table_if_not_exists.result'
> --- a/mysql-test/suite/rpl/r/rpl_create_tmp_table_if_not_exists.result	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_create_tmp_table_if_not_exists.result	2009-07-28
> 08:19:57 +0000
> @@ -0,0 +1,22 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +DROP DATABASE IF EXISTS mysqltest;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp(c1 int);
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp(c1 int);
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp1 LIKE tmp;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp1 LIKE tmp;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp2 SELECT * FROM tmp;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp2 SELECT * FROM tmp;
> +show binlog events from <binlog_start>;
> +Log_name	Pos	Event_type	Server_id	End_log_pos	Info
> +master-bin.000001	#	Query	#	#	DROP DATABASE IF EXISTS mysqltest
> +master-bin.000001	#	Query	#	#	use `test`; CREATE TEMPORARY TABLE IF NOT EXISTS
> tmp(c1 int)
> +master-bin.000001	#	Query	#	#	use `test`; CREATE TEMPORARY TABLE IF NOT EXISTS
> tmp(c1 int)
> +master-bin.000001	#	Query	#	#	use `test`; CREATE TEMPORARY TABLE IF NOT EXISTS tmp1
> LIKE tmp
> +master-bin.000001	#	Query	#	#	use `test`; CREATE TEMPORARY TABLE IF NOT EXISTS tmp1
> LIKE tmp
> +master-bin.000001	#	Query	#	#	use `test`; CREATE TEMPORARY TABLE IF NOT EXISTS tmp2
> SELECT * FROM tmp
> +master-bin.000001	#	Query	#	#	use `test`; CREATE TEMPORARY TABLE IF NOT EXISTS tmp2
> SELECT * FROM tmp
>
> === added file 'mysql-test/suite/rpl/t/rpl_create_if_not_exists.test'
> --- a/mysql-test/suite/rpl/t/rpl_create_if_not_exists.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_create_if_not_exists.test	2009-07-28 08:19:57 +0000
> @@ -0,0 +1,76 @@
> +# BUG#45574:
> +# SP: CREATE DATABASE|TABLE IF NOT EXISTS not binlogged if routine exists.
>   

> +#
> +#   There is an inconsistency with DROP DATABASE|TABLE IF EXISTS and CREATE
> +#   EVENT IF NOT EXISTS :DROP IF EXISTS statements are binlogged even if the
> +#   DB or TABLE does not exist, CREATE EVENT IF NOT EXISTS are binlogged even
> +#   if the event exists.  whereas CREATE DATEBASE|TABLE IF NOT EXISTS dose not
> +#   binlog when the DB or TABLE exists.  
>   
There is an inconsistency with DROP DATABASE|TABLE|EVENT IF EXISTS and CREATE
EVENT IF NOT EXISTS: DROP IF EXISTS statements are binlogged even if either the
DB, TABLE or event does not exist; CREATE EVENT IF NOT EXISTS is binlogged even
if the event exists. In contrast, CREATE DATEBASE|TABLE IF NOT EXISTS are not
binlogged when either the DB or TABLE exists.  

> +#
> +#   This problem caused some of the tests fails randomly on PB or PB2.
> +#
>   
This problem caused some of the tests to fail randomly on PB or PB2.
> +# Description: 
> +#   Fixed this bug by adding calls to write_bin_log in: 
> +#   mysql_create_db 
> +#   mysql_create_table_no_lock 
> +#   mysql_create_like_table 
> +#   create_table_from_items 
> +#
> +#   Test is implemented as follows: 
> +#   i) test each "CREATE IF NOT EXISTS" (DDL), found in MySQL 5.1 manual
> +#   exclude CREATE TEMPORARY TABLE, on existent objects; 
> +#
> +#  Note: 
> +#  rpl_create_tmp_table_if_not_exists.test tests CREATE TEMPORARY TABLE cases.
> +#
> +#  References:
> +#  http://dev.mysql.com/doc/refman/5.1/en/sql-syntax-data-definition.html
> +#
> +
> +source include/master-slave.inc;
> +disable_warnings;
> +DROP DATABASE IF EXISTS mysqltest;
> +
> +CREATE DATABASE IF NOT EXISTS mysqltest;
> +USE mysqltest;
> +CREATE TABLE IF NOT EXISTS t(c1 int);
> +CREATE TABLE IF NOT EXISTS t1 LIKE t;
> +CREATE TABLE IF NOT EXISTS t2 SELECT * FROM t;
> +CREATE EVENT IF NOT EXISTS e 
> +ON SCHEDULE AT CURRENT_TIMESTAMP + INTERVAL 1 HOUR 
> +DO SELECT now();
>   

> +save_master_pos;
> +
> +connection slave;
> +sync_with_master;
>   
replace this by sync_slave_with_master;
> +
> +#DROP database from slave.
> +#The database and all tables can be recreated in slave 
> +#if binlog of the second CREATE command is recorded and sent from master to slave.
> +DROP DATABASE mysqltest;
> +
> +connection master;
> +CREATE DATABASE IF NOT EXISTS mysqltest;
> +USE mysqltest;
> +CREATE TABLE IF NOT EXISTS t(c1 int);
> +CREATE TABLE IF NOT EXISTS t1 LIKE t;
> +CREATE TABLE IF NOT EXISTS t2 SELECT * FROM t;
> +CREATE EVENT IF NOT EXISTS e 
> +ON SCHEDULE AT CURRENT_TIMESTAMP + INTERVAL 1 HOUR 
> +DO SELECT now();
>   

> +save_master_pos;
> +
> +connection slave;
> +sync_with_master;
>   
See comment above;
> +SHOW TABLES in mysqltest;
> +#Executing time changes each test. this test do't care the executing time.
> +#It is replaced by '-'.
> +replace_column 6 -;
> +SHOW EVENTS in mysqltest;
> +
> +
> +connection master;
> +DROP DATABASE IF EXISTS mysqltest;
>   

> +save_master_pos;
> +sync_slave_with_master;
>   
See comment above;
> +source include/master-slave-end.inc;
>
> === added file 'mysql-test/suite/rpl/t/rpl_create_tmp_table_if_not_exists.test'
> --- a/mysql-test/suite/rpl/t/rpl_create_tmp_table_if_not_exists.test	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_create_tmp_table_if_not_exists.test	2009-07-28
> 08:19:57 +0000
> @@ -0,0 +1,32 @@
> +# BUG#45574:
>   
Copy the explanation from the other test case.
> +#
> +#   Test is implemented as follows:
> +#
> +#       i) test each "CREATE TEMPORARY TABLE IF EXISTS" (DDL), found in MySQL
> +#       5.1 manual, on existent objects; 
> +#       ii) show binlog events; 
> +#
> +#  Note: 
> +#  rpl_create_if_not_exists.test tests other cases.
> +#
> +#  References:
> +#  http://dev.mysql.com/doc/refman/5.1/en/sql-syntax-data-definition.html
> +#
> +
>   

> +source include/master-slave.inc;
> +#CREATE TEMPORARY TABLE statements are not binlogged in row mode,
> +#So it must be test by itself.
> +source include/have_binlog_format_mixed_or_statement.inc;
> +disable_warnings;
>   

> +
> +DROP DATABASE IF EXISTS mysqltest;
> +
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp(c1 int);
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp(c1 int);
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp1 LIKE tmp;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp1 LIKE tmp;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp2 SELECT * FROM tmp;
> +CREATE TEMPORARY TABLE IF NOT EXISTS tmp2 SELECT * FROM tmp;
> +source include/show_binlog_events.inc;
> +
> +source include/master-slave-end.inc;
>
> === modified file 'sql/sql_db.cc'
> --- a/sql/sql_db.cc	2009-05-30 13:32:28 +0000
> +++ b/sql/sql_db.cc	2009-07-28 08:19:57 +0000
> @@ -658,10 +658,8 @@ int mysql_create_db(THD *thd, char *db, 
>      }
>      push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
>  			ER_DB_CREATE_EXISTS, ER(ER_DB_CREATE_EXISTS), db);
> -    if (!silent)
> -      my_ok(thd);
>      error= 0;
> -    goto exit;
> +    goto not_silent;
>    }
>    else
>    {
> @@ -698,7 +696,8 @@ int mysql_create_db(THD *thd, char *db, 
>        happened.  (This is a very unlikely senario)
>      */
>    }
> -  
> +
> +not_silent:
>    if (!silent)
>    {
>      char *query;
>   

> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2009-06-04 23:30:08 +0000
> +++ b/sql/sql_insert.cc	2009-07-28 08:19:57 +0000
> @@ -3376,6 +3376,8 @@ static TABLE *create_table_from_items(TH
>        push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
>                            ER_TABLE_EXISTS_ERROR, ER(ER_TABLE_EXISTS_ERROR),
>                            create_table->table_name);
> +      hooks->prelock(&(create_table->table), 1);
> +      hooks->postlock(&(create_table->table), 1);
>        DBUG_RETURN(create_table->table);
>      }
>   
I am not sure if this is the right approach.
What part of you test case covers this?
>  
> @@ -3556,7 +3558,8 @@ select_create::prepare(List<Item> &value
>        TABLE const *const table = *tables;
>        if (thd->current_stmt_binlog_row_based  &&
>            !table->s->tmp_table &&
> -          !ptr->get_create_info()->table_existed)
> +          (!ptr->get_create_info()->table_existed ||
> +           (ptr->get_create_info()->options &
> HA_LEX_CREATE_IF_NOT_EXISTS)))
>        {
>          ptr->binlog_show_create_table(tables, count);
>        }
>   
ok.
> === modified file 'sql/sql_table.cc'
> --- a/sql/sql_table.cc	2009-06-02 09:07:17 +0000
> +++ b/sql/sql_table.cc	2009-07-28 08:19:57 +0000
> @@ -3432,6 +3432,41 @@ void sp_prepare_create_field(THD *thd, C
>  
>  
>  /*
> +  Write CREATE TABLE binlog
> +
> +  SYNOPSIS
> +    write_create_table_bin_log()
> +    thd               Thread object
> +    create_info       Create information
> +    internal_tmp_table  Set to 1 if this is an internal temporary table
> +
> +  DESCRIPTION
> +    This function only is called in mysql_create_table_no_lock and
> +    mysql_create_table
> +
> +  RETURN VALUES
> +    NONE
> + */
> +static inline void write_create_table_bin_log(THD *thd,
> +                                              const HA_CREATE_INFO *create_info,
> +                                              bool internal_tmp_table)
> +{
> +  /*
> +    Don't write statement if:
> +    - It is an internal temporary table,
> +    - Row-based logging is used and it we are creating a temporary table, or
> +    - The binary log is not open.
> +    Otherwise, the statement shall be binlogged.
> +   */
> +  if (!internal_tmp_table &&
> +      (!thd->current_stmt_binlog_row_based ||
> +       (thd->current_stmt_binlog_row_based &&
> +        !(create_info->options & HA_LEX_CREATE_TMP_TABLE))))
> +    write_bin_log(thd, TRUE, thd->query, thd->query_length);
> +}
> +
> +
> +/*
>    Create a table
>  
>    SYNOPSIS
> @@ -3694,6 +3729,7 @@ bool mysql_create_table_no_lock(THD *thd
>                            ER_TABLE_EXISTS_ERROR, ER(ER_TABLE_EXISTS_ERROR),
>                            alias);
>        error= 0;
>   
Do you think it is possible to re-organize the code in order to avoid
the function above and keep just one call to write_bin_log in the
mysql_create_table_no_lock?
> +      write_create_table_bin_log(thd, create_info, internal_tmp_table);
>        goto err;
>   
ok.
>      }
>      my_error(ER_TABLE_EXISTS_ERROR, MYF(0), alias);
> @@ -3814,18 +3850,7 @@ bool mysql_create_table_no_lock(THD *thd
>      thd->thread_specific_used= TRUE;
>    }
>  
> -  /*
> -    Don't write statement if:
> -    - It is an internal temporary table,
> -    - Row-based logging is used and it we are creating a temporary table, or
> -    - The binary log is not open.
> -    Otherwise, the statement shall be binlogged.
> -   */
> -  if (!internal_tmp_table &&
> -      (!thd->current_stmt_binlog_row_based ||
> -       (thd->current_stmt_binlog_row_based &&
> -        !(create_info->options & HA_LEX_CREATE_TMP_TABLE))))
> -    write_bin_log(thd, TRUE, thd->query, thd->query_length);
> +  write_create_table_bin_log(thd, create_info, internal_tmp_table);
>    error= FALSE;
>  unlock_and_end:
>    VOID(pthread_mutex_unlock(&LOCK_open));
> @@ -3841,6 +3866,7 @@ warn:
>                        ER_TABLE_EXISTS_ERROR, ER(ER_TABLE_EXISTS_ERROR),
>                        alias);
>    create_info->table_existed= 1;		// Mark that table existed
> +  write_create_table_bin_log(thd, create_info, internal_tmp_table);
>    goto unlock_and_end;
>  }
>  
>   
ok.
> @@ -3892,6 +3918,7 @@ bool mysql_create_table(THD *thd, const 
>                              table_name);
>          create_info->table_existed= 1;
>          result= FALSE;
> +        write_create_table_bin_log(thd, create_info, internal_tmp_table);
>        }
>        else
>        {
> @@ -5231,6 +5258,7 @@ bool mysql_create_like_table(THD* thd, T
>      goto err;	    /* purecov: inspected */
>    }
>   
Why do you need to call it from here? Note that the function
mysql_create_table_no_lock will still be called. Shouldn't it
be responsible for calling write_create_table_bin_log in this case?
Is there a corner case? If so, add some comments.
>  
> +binlog:
>    DBUG_EXECUTE_IF("sleep_create_like_before_binlogging", my_sleep(6000000););
>  
>    /*
> @@ -5305,6 +5333,7 @@ table_exists:
>      push_warning(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
>  		 ER_TABLE_EXISTS_ERROR,warn_buff);
>      res= FALSE;
> +    goto binlog;
>    }
>    else
>      my_error(ER_TABLE_EXISTS_ERROR, MYF(0), table_name);
>
>   
Please, move this part of code to a place before the label binlog
and change whatever is necessary in the logic to do that.
> ------------------------------------------------------------------------
>
>
>   

Thread
bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:2941) Bug#45574Li-Bing.Song28 Jul
  • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:2941)Bug#45574Alfranio Correia30 Jul
    • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:2941)Bug#45574Libing Song31 Jul