Hi!
On Sep 04, Georgi Kodinov wrote:
> #At file:///home/kgeorge/mysql/bzr/B32124-5.0-bugteam/
>
> 2672 Georgi Kodinov 2008-09-04
> Bug #32124: crash if prepared statements refer to variables in the where
> clause
>
> The code to get read the value of a system variable was extracting its value
> on PREPARE stage and was substituting the value (as a constant) into the parse
> tree.
> Note that this must be a reversible transformation, i.e. it must be reversed
> before
> each re-execution.
> Unfortunately this cannot be reliably done using the current code, because
> there are
> other non-reversible source tree transformations that can interfere with this
> reversible transformation.
> Fixed by not resolving the value at PREPARE, but at EXECUTE (as the rest of the
>
> functions operate). Added a cache of the value (so that it's constant
> throughout
> the execution of the query).
> Updated an obsolete related test suite (variables-big) and the code to test the
>
> result type of system variables (as per bug 74).
Looks good now.
I have a couple of questions, see below. And there's one minor problem
with the test case.
> === modified file 'mysql-test/r/ps_11bugs.result'
> --- a/mysql-test/r/ps_11bugs.result 2006-10-04 15:19:23 +0000
> +++ b/mysql-test/r/ps_11bugs.result 2008-09-04 12:02:56 +0000
> @@ -162,4 +162,31 @@ a b
> 12 NULL
> drop table t1;
> drop table t2;
> +CREATE TABLE t1 (a INT);
> +PREPARE stmt FROM 'select 1 from `t1` where `a` = any (select (@@tmpdir))';
> +EXECUTE stmt;
> +1
> +DEALLOCATE PREPARE stmt;
> +DROP TABLE t1;
> +CREATE TABLE t2 (a INT PRIMARY KEY);
> +INSERT INTO t2 VALUES (400000), (400001);
> +SET @@sort_buffer_size=400000;
> +CREATE FUNCTION p1(i INT) RETURNS INT
> +BEGIN
> +SET @@sort_buffer_size= i;
> +RETURN i + 1;
> +END|
> +SELECT * FROM t2 WHERE a = @@sort_buffer_size AND p1(@@sort_buffer_size + 1) > a
> - 1;
> +a
> +400000
> +DROP TABLE t2;
> +SELECT CONCAT(@@sort_buffer_size);
> +CONCAT(@@sort_buffer_size)
> +400001
> +SELECT LEFT("12345", @@tmpdir);
> +LEFT("12345", @@tmpdir)
> +
> +Warnings:
> +Warning 1292 Truncated incorrect INTEGER value:
> '/home/kgeorge/mysql/bzr/B32124-5.0-bugteam/mysql-test/var/tmp'
Please, use a string variable that doesn't include the path or anything
specific for a system or a build (like hostname or version or build comment).
You can use, for example, @@date_format or @@ft_boolean_syntax.
> +SET @@sort_buffer_size=DEFAULT;
> End of 5.0 tests.
>
> === modified file 'sql/item_func.cc'
> --- a/sql/item_func.cc 2008-07-30 11:07:37 +0000
> +++ b/sql/item_func.cc 2008-09-04 12:02:56 +0000
> @@ -4778,30 +4778,228 @@ Item_func_get_system_var::
> Item_func_get_system_var(sys_var *var_arg, enum_var_type var_type_arg,
> LEX_STRING *component_arg, const char *name_arg,
> size_t name_len_arg)
> - :var(var_arg), var_type(var_type_arg), component(*component_arg)
> + :var(var_arg), var_type(var_type_arg), component(*component_arg),
> + cache_present(false)
> {
> /* set_name() will allocate the name */
> set_name(name_arg, name_len_arg, system_charset_info);
> }
>
>
> -bool
> -Item_func_get_system_var::fix_fields(THD *thd, Item **ref)
> +void Item_func_get_system_var::fix_length_and_dec()
> {
> - Item *item;
> - DBUG_ENTER("Item_func_get_system_var::fix_fields");
> + maybe_null=0;
> + decimals=NOT_FIXED_DEC;
> + max_length=MAX_BLOB_WIDTH;
you don't need to set decimals and max_length here, you do it below in
the switch.
> + if (var->check_type(var_type))
> + {
> + if (var_type != OPT_DEFAULT)
> + {
> + my_error(ER_INCORRECT_GLOBAL_LOCAL_VAR, MYF(0),
> + var->name, var_type == OPT_GLOBAL ? "SESSION" : "GLOBAL");
> + return;
> + }
> + /* As there was no local variable, return the global value */
> + var_type= OPT_GLOBAL;
> + }
> +
> + switch (var->show_type())
> + {
> + case SHOW_LONG:
> + case SHOW_INT:
> + unsigned_flag= TRUE;
> + max_length= MY_INT64_NUM_DECIMAL_DIGITS;
> + decimals=0;
> + break;
> + case SHOW_HA_ROWS:
> + case SHOW_LONGLONG:
> + unsigned_flag= FALSE;
> + max_length= MY_INT64_NUM_DECIMAL_DIGITS;
> + decimals=0;
> + break;
> + case SHOW_CHAR:
> + collation.set(system_charset_info, DERIVATION_SYSCONST);
> + max_length= MAX_BLOB_WIDTH;
> + break;
> + case SHOW_MY_BOOL:
> + unsigned_flag= TRUE;
> + max_length= 1;
> + decimals=0;
> + break;
> + default:
> + my_error(ER_VAR_CANT_BE_READ, MYF(0), name);
> + break;
> + }
> +}
> +
> +String* Item_func_get_system_var::val_str(String* str)
> +{
> + THD *thd= current_thd;
> +
> + if (cache_present && thd->query_id == used_query_id)
> + return &cached_strval;
> +
> + switch (var->show_type())
> + {
> + case SHOW_CHAR:
> + {
> + pthread_mutex_lock(&LOCK_global_system_variables);
> + char *cptr= (char*) var->value_ptr(thd, var_type, &component);
> + if (cptr)
> + {
> + if (str->copy(cptr, strlen(cptr), collation.collation))
> + str= NULL;
> + }
> + else
> + {
> + null_value= TRUE;
> + str= NULL;
> + }
> + pthread_mutex_unlock(&LOCK_global_system_variables);
> + break;
> + }
> +
> + case SHOW_INT:
> + case SHOW_LONG:
> + case SHOW_LONGLONG:
> + case SHOW_HA_ROWS:
> + case SHOW_MY_BOOL:
> + str->set (val_int(), collation.collation);
> + break;
> +
> + default:
> + my_error(ER_VAR_CANT_BE_READ, MYF(0), name);
> + str= NULL;
> + break;
> + }
> +
> + if (str)
> + {
Why do you have "if (str)" ? If the value was evaluated to NULL it's
still a valid value that should be cached. I think you should execute
the code below unconditionally.
> + cache_present= true;
> + used_query_id= thd->query_id;
> + if (cached_strval.copy(*str))
Two String::copy() ? Why not to use cached_strval directly in the
switch() above ?
> + cache_present= false;
> + }
> + return str;
> +}
> +
Regards / Mit vielen Grüßen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring
