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;
> }
> ;
>
>
>
>
>
