MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:August 17 2010 8:43pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)
WL#5370
View as plain text  
* Li-Bing.Song@stripped <Li-Bing.Song@stripped> [10/08/17 14:58]:
>  3479 Li-Bing.Song@stripped	2010-08-17
>       WL#5370 Keep forward-compatibility when changing
>               'CREATE TABLE IF NOT EXISTS ... SELECT' behaviour

OK to push, please see below.

> --- a/mysql-test/extra/rpl_tests/rpl_stm_create_if_not_exists.test	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_stm_create_if_not_exists.test	2010-08-17
> 10:05:41 +0000
> @@ -0,0 +1,231 @@
> +--echo
> +--echo
> +connection master;
> +
> +if ($is_temporary)
> +{
> +  --let $_temporary=TEMPORARY
> +}

The tests look very nice.


> === modified file 'mysql-test/r/commit_1innodb.result'
> --- a/mysql-test/r/commit_1innodb.result	2010-07-20 17:36:15 +0000
> +++ b/mysql-test/r/commit_1innodb.result	2010-08-17 10:05:41 +0000
> @@ -826,8 +826,6 @@ SUCCESS
>  #
>  # Sic: no table is created.
>  create table if not exists t2 (a int) select 6 union select 7;
> -Warnings:
> -Note	1050	Table 't2' already exists
>  # Sic: first commits the statement, and then the transaction.
>  call p_verify_status_increment(4, 4, 4, 4);
>  SUCCESS

Why is the warning suppressed? Why does it show up in SBR only?

> 
> === modified file 'mysql-test/r/trigger.result'
> --- a/mysql-test/r/trigger.result	2010-07-08 18:46:26 +0000
> +++ b/mysql-test/r/trigger.result	2010-08-17 10:05:41 +0000
> @@ -1824,11 +1824,8 @@ Note	1050	Table 'v1' already exists
>  set @id=last_insert_id();
>  select * from t1;
>  id	operation
> -1	CREATE TABLE ... SELECT, inserting a new key
>  select * from t1_op_log;
>  operation
> -Before INSERT, new=CREATE TABLE ... SELECT, inserting a new key
> -After INSERT, new=CREATE TABLE ... SELECT, inserting a new key
>  truncate t1_op_log;
>  create table if not exists v1 replace
>  select @id, "CREATE TABLE ... REPLACE SELECT, deleting a duplicate key";
> @@ -1836,13 +1833,8 @@ Warnings:
>  Note	1050	Table 'v1' already exists
>  select * from t1;
>  id	operation
> -1	CREATE TABLE ... REPLACE SELECT, deleting a duplicate key
>  select * from t1_op_log;
>  operation
> -Before INSERT, new=CREATE TABLE ... REPLACE SELECT, deleting a duplicate key
> -Before DELETE, old=CREATE TABLE ... SELECT, inserting a new key
> -After DELETE, old=CREATE TABLE ... SELECT, inserting a new key
> -After INSERT, new=CREATE TABLE ... REPLACE SELECT, deleting a duplicate key
>  truncate t1;
>  truncate t1_op_log;
>  insert into v1 (id, operation)

You could explain in trigger.result changeset comments why 
these triggers are no longer executed (we no longer execute CTS if
the underlying table is a view), that would save me
(and anyone looking at this patch later on) a few moments.

> === modified file 'mysql-test/suite/rpl/t/rpl_create_if_not_exists.test'
> --- a/mysql-test/suite/rpl/t/rpl_create_if_not_exists.test	2010-01-16 07:44:24 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_create_if_not_exists.test	2010-08-17 10:05:41 +0000
> @@ -95,7 +95,9 @@ CREATE TEMPORARY TABLE t2(c1 INTEGER);
>  INSERT INTO t1 VALUES(1);
>  INSERT INTO t2 VALUES(1);
>  

Please add a comment why you disable warnings.

> +--disable_warnings
>  CREATE TABLE IF NOT EXISTS t1(c1 INTEGER) SELECT c1 FROM t3;
> +--enable_warnings
>  CREATE TABLE t2(c1 INTEGER) SELECT c1 FROM t3;
>  
>  # In these two statements, t1 and t2 are the temporary table. there is only
>  
> +int select_insert::write_to_binlog(bool is_trans, int errcode)
> +{
> +  /* It is only for statement mode */
> +  if (thd->current_stmt_binlog_row_based)
> +    return 0;
> +
> +  return thd->binlog_query(THD::ROW_QUERY_TYPE,
> +                        thd->query(), thd->query_length(),
> +                        is_trans, FALSE, errcode);
> +}

Please fix alignment of this call.

> +
> +    if (query.real_alloc(1024))
> +      return 1;

Why 1024? In theory it can contain MAX_FIELDS *
(TABLE_DBKEY_LENGTH + MAX_FIELD_NAME) bytes.
In other words, a lot :)

There is a silly constant STRING_BUFFER_USUAL_SIZE,
you could use it instead of 1024. It would
at least make it clear to the reader that 1024
has no special meaning and you're not making the
code that follows resilient to OOM.

> +
> +    if (thd->lex->ignore)
> +      query.append(STRING_WITH_LEN("INSERT IGNORE INTO `"));
> +    else if (thd->lex->duplicates == DUP_REPLACE)
> +      query.append(STRING_WITH_LEN("REPLACE INTO `"));
> +    else
> +      query.append(STRING_WITH_LEN("INSERT INTO `"));
> +
> +    query.append(create_table->db, strlen(create_table->db));
> +    query.append(STRING_WITH_LEN("`.`"));
> +    query.append(create_info->alias, strlen(create_info->alias));
> +    query.append(STRING_WITH_LEN("` "));

> +    /*
> +      The insert items.
> +      Field is the the rightmost columns that the rows are inster in.
> +    */
> +    query.append(STRING_WITH_LEN("("));
> +    for (Field **f= field ; *f ; f++)
> +    {
> +      if (f != field)
> +        query.append(STRING_WITH_LEN(","));
> +
> +      query.append(STRING_WITH_LEN("`"));
> +      query.append((*f)->field_name, strlen((*f)->field_name));
> +      query.append(STRING_WITH_LEN("`"));

> +    }
> +    query.append(STRING_WITH_LEN(") "));
> +
> +    /* The SELECT clause*/
> +    DBUG_ASSERT(thd->lex->create_select_pos);
> +    if (thd->lex->create_select_in_comment)
> +      query.append(STRING_WITH_LEN("/*!"));
> +    if (thd->lex->create_select_start_with_brace)
> +      query.append(STRING_WITH_LEN("("));
> +    if (query.append(thd->query() + thd->lex->create_select_pos,
> +                 thd->query_length() - thd->lex->create_select_pos))
> +      return 1;

I agree it's safe to only check the last append because
append() doesn't impact String object if it fails.

> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h	2010-07-29 03:00:57 +0000
> +++ b/sql/sql_lex.h	2010-08-17 10:05:41 +0000
> @@ -1817,6 +1817,23 @@ typedef struct st_lex : public Query_tab
>    */
>    bool protect_against_global_read_lock;
>  
> +  /*
> +    The following to variables are used in 'CREATE TABLE IF NOT EXISTS ...

s/to/two
> +    SELECT' statement. They are used to binlog the statement.
> +
> +    create_select_start_with_brace will be set if there is a '(' before
> +    the first SELECT clause
> +
> +    create_select_pos records the relative position of the SELECT clause
> +    in the whole statement.
> +
> +    create_select_in_comment will be set if SELECT keyword is in conditional
> +    comment.
> +   */
> +  bool create_select_start_with_brace;
> +  uint create_select_pos;
> +  bool create_select_in_comment;
> +
>    st_lex();
>  
>    virtual ~st_lex()
> 
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy	2010-07-19 14:30:34 +0000
> +++ b/sql/sql_yacc.yy	2010-08-17 10:05:41 +0000
> @@ -3881,17 +3881,26 @@ create2a:
>            create3 {}
>          |  opt_partitioning
>             create_select ')'
> -           { Select->set_braces(1);}
> +           {
> +             Select->set_braces(1);
> +             Lex->create_select_start_with_brace= true;
> +           }

Use TRUE instead of true.

>             union_opt {}
>          ;
>  
>  create3:
>            /* empty */ {}
>          | opt_duplicate opt_as create_select
> -          { Select->set_braces(0);}
> +          {
> +            Select->set_braces(0);
> +            Lex->create_select_start_with_brace= false;

Use FALSE instead of false.


-- 
Thread
bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479) WL#5370Li-Bing.Song17 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)WL#5370He Zhenxing17 Aug
    • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)WL#5370Libing Song17 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)WL#5370Konstantin Osipov17 Aug
    • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)WL#5370Libing Song18 Aug
      • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)WL#5370Konstantin Osipov18 Aug