List:Commits« Previous MessageNext Message »
From:Luís Soares Date:September 22 2010 11:38pm
Subject:Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3136) Bug#56678
View as plain text  
Hi Jon,

  Thanks for fixing this. Nice work.

  You wrote a very nice and clear bug description in the commit
  message. So double thanks. Please, find below my review comments.


STATUS
------

  Approved (after considering RC1 below).

REQUIRED CHANGES
----------------

   RC1. In create_insert_stmt_from_insert_delayed, there is this
        change:

+  /* Make a copy of thd->query() and then remove the "DELAYED" keyword */
+  if (buf->append(thd->query()) ||
+      buf->replace(thd->lex->keyword_delayed_begin_offset, 7, 0))

        Why not replace "DELAYED " instead of just "DELAYED", ie,
        use 8 instead of 7 as the parameter "uint32 arg_length"
        to String::replace ? Then it would become:

+  /* Make a copy of thd->query() and then remove the "DELAYED" keyword */
+  if (buf->append(thd->query()) ||
+      buf->replace(thd->lex->keyword_delayed_begin_offset, 8, 0))

       So my question is whether there is any hindrance removing
       the trailing " " following "DELAYED" keyword or not ? My
       guess is not.

       Anyway, if you agree, then I think we should remove it -
       use 8 instead of 7 (yes, it is a little cosmetics... yes,
       I think it was an issue in the original patch for
       BUG#54579 as well... yes, we save little on disk space but
       it's something :P ).

REQUESTS
--------
 n/a

SUGGESTIONS
-----------
 n/a

OBSERVATIONS
------------

  O1. FYI, I patched mysql-5.5-bugfixing with your proposed
      patch and:

       1. ran MTR: no failures
       2. ran --valgrind on binlog_unsafe: no failure

DETAILS
-------
 n/a

Regards,
Luís Soares


On 09/13/2010 09:25 AM, Jon Olav Hauglid wrote:
> #At file:///export/home/x/mysql-5.5-runtime-bug56678/ based on
> revid:jon.hauglid@stripped
> 
>  3136 Jon Olav Hauglid	2010-09-13
>       Bug #56678 Valgrind warnings from binlog.binlog_unsafe
>       
>       After the patch for Bug#54579, multi inserts done with INSERT DELAYED
>       are binlogged as normal INSERT. During processing of the statement,
>       a new query string without the DELAYED keyword is made. The problem
>       was that this new string was incorrectly made when the INSERT DELAYED
>       was part of a prepared statement - data was read outside the allocated
>       buffer.
>       
>       The reason for this bug was that a pointer to the position of the
>       DELAYED keyword inside the query string was stored when parsing the
>       statement. This pointer was then later (at runtime) used (via pointer
>       subtraction) to find the number of characters to skip when making a
>       new query string without DELAYED. But when the statement was re-executed
>       as part of a prepared statement, the original pointer would be invalid
>       and the pointer subtraction would give a wrong/random result.
>       
>       This patch fixes the problem by instead storing the number of characters
>       to skip at parse time. This value will not depend on the memory position
>       of the query string at runtime and therefore not give wrong results
>       when the statement is executed in a prepared statement.
>       
>       This bug was a regression introduced by the patch for Bug#54579.
>       
>       No test case added as this bug is already covered by the existing
>       binlog.binlog_unsafe test case when running with valgrind.
> 
>     modified:
>       sql/sql_insert.cc
>       sql/sql_lex.h
>       sql/sql_yacc.yy
> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2010-09-01 13:12:42 +0000
> +++ b/sql/sql_insert.cc	2010-09-13 08:25:06 +0000
> @@ -634,14 +634,10 @@ bool open_and_lock_for_insert_delayed(TH
>  static int
>  create_insert_stmt_from_insert_delayed(THD *thd, String *buf)
>  {
> -  /* Append the part of thd->query before "DELAYED" keyword */
> -  if (buf->append(thd->query(),
> -                  thd->lex->keyword_delayed_begin - thd->query()))
> +  /* Make a copy of thd->query() and then remove the "DELAYED" keyword */
> +  if (buf->append(thd->query()) ||
> +      buf->replace(thd->lex->keyword_delayed_begin_offset, 7, 0))
>      return 1;
> -  /* Append the part of thd->query after "DELAYED" keyword */
> -  if (buf->append(thd->lex->keyword_delayed_begin + 7))
> -    return 1;
> -
>    return 0;
>  }
>  
> 
> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h	2010-09-01 13:12:42 +0000
> +++ b/sql/sql_lex.h	2010-09-13 08:25:06 +0000
> @@ -2355,12 +2355,12 @@ struct LEX: public Query_tables_list
>      This pointer is required to add possibly omitted DEFINER-clause to the
>      DDL-statement before dumping it to the binlog.
>  
> -    keyword_delayed_begin points to the begin of the DELAYED keyword in
> -    INSERT DELAYED statement.
> +    keyword_delayed_begin_offset is the offset to the beginning of the DELAYED
> +    keyword in INSERT DELAYED statement.
>    */
>    union {
>      const char *stmt_definition_begin;
> -    const char *keyword_delayed_begin;
> +    uint keyword_delayed_begin_offset;
>    };
>  
>    const char *stmt_definition_end;
> 
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy	2010-09-09 14:29:14 +0000
> +++ b/sql/sql_yacc.yy	2010-09-13 08:25:06 +0000
> @@ -10447,7 +10447,8 @@ insert_lock_option:
>          | LOW_PRIORITY  { $$= TL_WRITE_LOW_PRIORITY; }
>          | DELAYED_SYM
>          {
> -          Lex->keyword_delayed_begin= YYLIP->get_tok_start();
> +          Lex->keyword_delayed_begin_offset= (uint)(YYLIP->get_tok_start() -
> +                                                    YYTHD->query());
>            $$= TL_WRITE_DELAYED;
>          }
>          | HIGH_PRIORITY { $$= TL_WRITE; }
> @@ -10457,7 +10458,8 @@ replace_lock_option:
>            opt_low_priority { $$= $1; }
>          | DELAYED_SYM
>          {
> -          Lex->keyword_delayed_begin= YYLIP->get_tok_start();
> +          Lex->keyword_delayed_begin_offset= (uint)(YYLIP->get_tok_start() -
> +                                                    YYTHD->query());
>            $$= TL_WRITE_DELAYED;
>          }
>          ;
> 
> 
> 
> 
> 

Thread
bzr commit into mysql-5.5-runtime branch (jon.hauglid:3136) Bug#56678Jon Olav Hauglid13 Sep
  • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3136) Bug#56678Luís Soares23 Sep
    • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3136) Bug#56678Jon Olav Hauglid23 Sep