List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:April 14 2010 1:53am
Subject:Re: bzr commit into mysql-trunk-bugfixing branch (kostja:3015)
Bug#11918
View as plain text  
Hi Konstantin,

On 4/13/10 6:56 PM, Konstantin Osipov wrote:
> #At file:///opt/local/work/trunk-11918/ based on
> revid:vvaintroub@stripped
>
>   3015 Konstantin Osipov	2010-04-14
>        A fix for Bug#11918 "SP does not accept variables in LIMIT clause"
>
>        Allow stored procedure variables in LIMIT clause.
>        Only allow variables of INTEGER types.
>        Handle negative values by means of an implicit cast to UNSIGNED
>        (similarly to prepared statement placeholders).
>        Add tests.
>        Make sure replication works by not doing NAME_CONST substitution
>        for variables in LIMIT clause.

Sounds right. NAME_CONST seems to have been intended to preserve 
properties that does not apply to integers (character set, etc).

>        Add replication tests.
>       @ mysql-test/r/sp.result
>          Update results (Bug#11918).
>       @ mysql-test/suite/rpl/r/rpl_sp.result
>          Update results (Bug#11918).
>       @ mysql-test/suite/rpl/t/rpl_sp.test
>          Add a test case for Bug#11918.
>       @ mysql-test/t/sp.test
>          Add a test case for Bug#11918.
>       @ sql/item.cc
>          Mark sp variables in LIMIT clause (a hack for replication).
>       @ sql/item.h
>          Mark sp variables in LIMIT clause (a hack for replication).
>       @ sql/share/errmsg-utf8.txt
>          Add a new error message (a type mismatch for LIMIT
>          clause parameter).
>       @ sql/sp_head.cc
>          Binlog rewrite sp variables in LIMIT clause without NAME_CONST
>          substitution.
>       @ sql/sql_string.cc
>          Implement append_ulonglong method.
>       @ sql/sql_string.h
>          Declare append_ulonglong().
>       @ sql/sql_yacc.yy
>          Support stored procedure variables in LIMIT.
>
>      modified:
>        mysql-test/r/sp.result
>        mysql-test/suite/rpl/r/rpl_sp.result
>        mysql-test/suite/rpl/t/rpl_sp.test
>        mysql-test/t/sp.test
>        sql/item.cc
>        sql/item.h
>        sql/share/errmsg-utf8.txt
>        sql/sp_head.cc
>        sql/sql_string.cc
>        sql/sql_string.h
>        sql/sql_yacc.yy

[..]

> === modified file 'mysql-test/suite/rpl/t/rpl_sp.test'
> --- a/mysql-test/suite/rpl/t/rpl_sp.test	2010-02-11 16:02:21 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_sp.test	2010-04-13 21:56:19 +0000
> @@ -679,6 +679,56 @@ drop table t1, t2;
>   --error ER_SP_DOES_NOT_EXIST
>   drop function f1;
>
> +
> +--echo #
> +--echo # Bug #11918 Can't use a declared variable in LIMIT clause
> +--echo #
> +--disable_warnings
> +drop table if exists t1;
> +drop procedure if exists p1;
> +--enable_warnings
> +connection master;
> +-- source include/master-slave-reset.inc
> +connection default;
> +create table t1 (c1 int);
> +insert into t1 (c1) values
> +(1), (2), (3), (4), (5), (6), (7), (8), (9), (10);
> +
> +call mtr.add_suppression("Unsafe statement binlogged in statement format since
> BINLOG_FORMAT = STATEMENT");
> +
> +create procedure p1(p1 integer)
> +  delete from t1 limit p1;
> +
> +set @save_binlog_format=@@session.binlog_format;
> +set @@session.binlog_format=STATEMENT;
> +
> +--disable_warnings
> +call p1(NULL);
> +call p1(0);
> +call p1(1);
> +call p1(2);
> +call p1(3);
> +--enable_warnings
> +
> +select * from t1;
> +sync_slave_with_master;
> +connection slave;
> +select * from t1;
> +connection default;
> +--disable_warnings
> +call p1(-1);
> +--enable_warnings
> +select * from t1;
> +sync_slave_with_master;
> +connection slave;
> +select * from t1;
> +connection default;
> +
> +--echo # Cleanup
> +set @@session.binlog_format=@save_binlog_format;
> +drop table t1;
> +drop procedure p1;
> +
>   --echo # End of 5.5 tests.
>
>
>
> === modified file 'mysql-test/t/sp.test'
> --- a/mysql-test/t/sp.test	2010-03-18 10:38:29 +0000
> +++ b/mysql-test/t/sp.test	2010-04-13 21:56:19 +0000
> @@ -8599,4 +8599,95 @@ SELECT * FROM t1;
>   DROP TEMPORARY TABLE t1;
>   DROP PROCEDURE p1;
>
> +--echo #
> +--echo # Bug #11918 Can't use a declared variable in LIMIT clause
> +--echo #
> +--disable_warnings
> +drop table if exists t1;
> +drop procedure if exists p1;
> +--enable_warnings
> +create table t1 (c1 int);
> +insert into t1 (c1) values (1), (2), (3), (4), (5);
>
> +delimiter |;
> +create procedure p1()
> +begin
> +  declare a integer;
> +  declare b integer;
> +  select * from t1 limit a, b;
> +end|
> +delimiter ;|
> +--echo # How do we handle NULL limit values?
> +call p1();
> +drop table t1;
> +create table t1 (a int);
> +insert into t1 (a) values (1), (2), (3), (4), (5);
> +--echo #
> +--echo # Do we correctly resolve identifiers in LIMIT?
> +--echo # Since DROP and CREATE did not invalidate
> +--echo # the SP cache, we can't test until
> +--echo # we drop and re-create the procedure.
> +--echo #
> +--error ER_BAD_FIELD_ERROR
> +call p1();
> +--echo #
> +--echo # Drop and recreate the procedure, then repeat
> +--echo #
> +drop procedure p1;
> +delimiter |;
> +create procedure p1()
> +begin
> +  declare a integer;
> +  declare b integer;
> +  select * from t1 limit a, b;
> +end|
> +delimiter ;|
> +--echo # Stored procedure variables are resolved correctly in the LIMIT
> +call p1();
> +drop table t1;
> +create table t1 (c1 int);
> +insert into t1 (c1) values (1), (2), (3), (4), (5);
> +drop procedure p1;
> +--echo # Try to create a procedure that
> +--echo # refers to non-existing variables.
> +--error ER_SP_UNDECLARED_VAR
> +create procedure p1(p1 integer, p2 integer)
> +  select * from t1 limit a, b;
> +--echo #
> +--echo # Try to use data types not allowed in LIMIT
> +--echo #
> +--error ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +create procedure p1(p1 date, p2 date) select * from t1 limit p1, p2;
> +--error ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +create procedure p1(p1 integer, p2 float) select * from t1 limit p1, p2;
> +--error ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +create procedure p1(p1 integer, p2 char(1)) select * from t1 limit p1, p2;
> +--error ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +create procedure p1(p1 varchar(5), p2 char(1)) select * from t1 limit p1, p2;
> +--error ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +create procedure p1(p1 decimal, p2 decimal) select * from t1 limit p1, p2;
> +--error ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +create procedure p1(p1 double, p2 double) select * from t1 limit p1, p2;
> +
> +--echo #
> +--echo # Finally, test the valid case.
> +--echo #
> +create procedure p1(p1 integer, p2 integer)
> +  select * from t1 limit p1, p2;
> +
> +call p1(NULL, NULL);
> +call p1(0, 0);
> +call p1(0, -1);
> +call p1(-1, 0);
> +call p1(-1, -1);
> +call p1(0, 1);
> +call p1(1, 0);
> +call p1(1, 5);
> +call p1(3, 2);
> +
> +
> +--echo # Cleanup
> +drop table t1;
> +drop procedure p1;
> +
> +--echo # End of 5.5 test

Hum, please also add tests for functions and triggers.

> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2010-04-07 11:58:40 +0000
> +++ b/sql/item.cc	2010-04-13 21:56:19 +0000
> @@ -1186,7 +1186,9 @@ Item_splocal::Item_splocal(const LEX_STR
>                              enum_field_types sp_var_type,
>                              uint pos_in_q, uint len_in_q)
>     :Item_sp_variable(sp_var_name.str, sp_var_name.length),
> -   m_var_idx(sp_var_idx), pos_in_query(pos_in_q), len_in_query(len_in_q)
> +   m_var_idx(sp_var_idx),
> +   limit_clause_param(FALSE),
> +   pos_in_query(pos_in_q), len_in_query(len_in_q)
>   {
>     maybe_null= TRUE;
>
>
> === modified file 'sql/item.h'
> --- a/sql/item.h	2010-03-31 14:05:33 +0000
> +++ b/sql/item.h	2010-04-13 21:56:19 +0000
> @@ -1302,6 +1302,13 @@ class Item_splocal :public Item_sp_varia
>     Item_result m_result_type;
>     enum_field_types m_field_type;
>   public:
> +  /*
> +    Is this variable a parameter in LIMIT clause.

s/Is/If/

> +    Used only during NAME_CONST substitution, to not append
> +    NAME_CONST to the resulting query and thus not break
> +    the slave.
> +  */
> +  bool limit_clause_param;
>     /*
>       Position of this reference to SP variable in the statement (the
>       statement itself is in sp_instr_stmt::m_query).
>
> === modified file 'sql/share/errmsg-utf8.txt'
> --- a/sql/share/errmsg-utf8.txt	2010-03-19 08:29:12 +0000
> +++ b/sql/share/errmsg-utf8.txt	2010-04-13 21:56:19 +0000
> @@ -6327,3 +6327,6 @@ ER_LOCK_ABORTED
>
>   ER_DATA_OUT_OF_RANGE 22003
>     eng "%s value is out of range in '%s'"
> +
> +ER_WRONG_SPVAR_TYPE_IN_LIMIT
> +  eng "A variable of a non-integer type in LIMIT clause"

I would add "non-integer based type".

> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc	2010-04-07 11:58:40 +0000
> +++ b/sql/sp_head.cc	2010-04-13 21:56:19 +0000
> @@ -978,11 +978,20 @@ subst_spvars(THD *thd, sp_instr *instr,
>       res|= qbuf.append(cur + prev_pos, (*splocal)->pos_in_query - prev_pos);
>       prev_pos= (*splocal)->pos_in_query + (*splocal)->len_in_query;
>
> +    res|= (*splocal)->fix_fields(thd, (Item **) splocal);

Hum.. the variables are only substituted if the instruction is a 
statement. But there are other type of instructions that might cause a 
query to be executed. Something like:

	DECLARE foo, cnt INT UNSIGNED DEFAULT 1;
	SET foo = (SELECT MIN(a) FROM t1 LIMIT cnt);

I think that fix_fields won't be called for the variable. We need to 
investigate under which circumstances it might be necessary to 
explicitly "fix_fields" the variables.

> +    if (res)
> +      break;
> +
> +    if ((*splocal)->limit_clause_param)
> +    {
> +      res|= qbuf.append_ulonglong((*splocal)->val_uint());

Return value ignored.

> +      continue;
> +    }
> +
>       /* append the spvar substitute */
>       res|= qbuf.append(STRING_WITH_LEN(" NAME_CONST('"));
>       res|= qbuf.append((*splocal)->m_name.str, (*splocal)->m_name.length);
>       res|= qbuf.append(STRING_WITH_LEN("',"));
> -    res|= (*splocal)->fix_fields(thd, (Item **) splocal);
>
>       if (res)
>         break;
>
> === modified file 'sql/sql_string.cc'
> --- a/sql/sql_string.cc	2010-02-24 09:15:34 +0000
> +++ b/sql/sql_string.cc	2010-04-13 21:56:19 +0000
> @@ -405,6 +405,16 @@ bool String::append(const char *s)
>   }
>
>
> +
> +bool String::append_ulonglong(ulonglong val)
> +{
> +  if (realloc(str_length+MAX_BIGINT_WIDTH+2))
> +    return TRUE;
> +  char *end= (char*) longlong10_to_str(val, (char*) Ptr + str_length, 10);

Probably harmless this way, but shouldn't we be using the character set 
provided conversion function? (similar to String::set_int).

> +  str_length= end - Ptr;
> +  return FALSE;
> +}
> +
>   /*
>     Append a string in the given charset to the string
>     with character set recoding
>
> === modified file 'sql/sql_string.h'
> --- a/sql/sql_string.h	2010-03-31 14:05:33 +0000
> +++ b/sql/sql_string.h	2010-04-13 21:56:19 +0000
> @@ -267,6 +267,7 @@ public:
>     bool append(const char *s);
>     bool append(const char *s,uint32 arg_length);
>     bool append(const char *s,uint32 arg_length, CHARSET_INFO *cs);
> +  bool append_ulonglong(ulonglong val);
>     bool append(IO_CACHE* file, uint32 arg_length);
>     bool append_with_prefill(const char *s, uint32 arg_length,
>   			   uint32 full_length, char fill_char);
>
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy	2010-03-31 14:05:33 +0000
> +++ b/sql/sql_yacc.yy	2010-04-13 21:56:19 +0000
> @@ -9830,7 +9830,40 @@ limit_options:
>           ;
>
>   limit_option:
> -        param_marker
> +        ident
> +        {
> +          Item_splocal *splocal;
> +          THD *thd= YYTHD;
> +          LEX *lex= thd->lex;
> +          Lex_input_stream *lip=&  thd->m_parser_state->m_lip;
> +          sp_variable_t *spv;
> +          sp_pcontext *spc = lex->spcont;
> +          if (spc && (spv = spc->find_variable(&$1)))
> +          {
> +            splocal= new (thd->mem_root)
> +              Item_splocal($1, spv->offset, spv->type,
> +                  lip->get_tok_start() - lex->sphead->m_tmp_query,
> +                  lip->get_ptr() - lip->get_tok_start());
> +            if (splocal == NULL)
> +              MYSQL_YYABORT;
> +#ifndef DBUG_OFF
> +            splocal->m_sp= lex->sphead;
> +#endif
> +            lex->safe_to_cache_query=0;
> +          }
> +          else
> +          {
> +            my_error(ER_SP_UNDECLARED_VAR, MYF(0), $1.str);
> +            MYSQL_YYABORT;
> +          }
> +          if (splocal->type() != Item::INT_ITEM)

It allows integer based types, but it's okay i guess (as listed in 
sp_map_item_type).

> +          {
> +            my_error(ER_WRONG_SPVAR_TYPE_IN_LIMIT, MYF(0));
> +            MYSQL_YYABORT;
> +          }
> +          splocal->limit_clause_param= TRUE;
> +          $$= splocal;
> +        } | param_marker

New line after the closing brace.

>           {
>             ((Item_param *) $1)->limit_clause_param= TRUE;
>           }
>

Otherwise, looks good! Thanks!

Regards,

Davi Arnaut

Thread
bzr commit into mysql-trunk-bugfixing branch (kostja:3015) Bug#11918Konstantin Osipov13 Apr
  • Re: bzr commit into mysql-trunk-bugfixing branch (kostja:3015)Bug#11918Davi Arnaut14 Apr