From: Libing Song
Date: August 18 2010 5:46am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3479)
 WL#5370
List-Archive: http://lists.mysql.com/commits/116026
Message-Id: <1282110408.1625.119.camel@my-laptop>
MIME-Version: 1.0
Content-Type: text/plain; CHARSET=US-ASCII
Content-Transfer-Encoding: 7BIT

Hi Kostja,
Thanks for your review.

[snip]
> 
> > === 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?
hmm, I forgot to correct it, it derived from the original patch.
> 
> > 
> > === 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.
ok.
> 
> > === 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.
It seems no reason to disable the warning. 
I will enable the warning.
> 
> > +--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 :)
> 
The pre-allocation intend to reduce reallocating memory
when executing query.append().
So I will calculate the real memory the query need.

> 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
OK. It should be three.
> > +    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.
OK.
> 
> >             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.
OK.
> 
> 
> -- 
> 

-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================