List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:October 6 2010 6:29am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3213) Bug#57095
View as plain text  
Hi, Evgeny.

Thanks for the patch. It seems to me that there is several ways this bug 
can be fixed.  (See my inline comments)  Do you view this as the ideal 
way, or have you chosen it because it is the least risky, given that we 
are trying to stabilize 5.5?

On 10/01/2010 01:38 PM, Evgeny Potemkin wrote:
> #At file:///work/bzrroot/57095-bug-5.5-bugfixing/ based on
> revid:jon.hauglid@stripped
>
>   3213 Evgeny Potemkin	2010-10-01
>        Bug#57095: Wrongly chosen expression cache type led to a wrong result.
>        The coalesce function returned DATETIME type due to a DATETIME argument, but
>        since it's not a date/time function it can't return correct int value for

It seems to me that it should not be very hard to make coalesce be able 
to return the correct int value.  I would think it would just be to pass 
on the int value from its argument.  Is there some inherent reason for 
not doing this?

Do we have the same issue with other functions? E.g. CASE?

>        it. Nevertheless Item_datetime_cache was chosen to cache coalesce's result
>        and that led to a wrong result.
>
>        Now Item_datetime_cache is used only for those function that could return
>        correct int representation of DATETIME values.
>       @ mysql-test/r/type_datetime.result
>          Added a test case for the bug#57095.
>       @ mysql-test/t/type_datetime.test
>          Added a test case for the bug#57095.
>       @ sql/item.cc
>          Bug#57095: Wrongly chosen expression cache type led to a wrong result.
>          Now Item_datetime_cache is used only for those function that could return
>          correct int representation of DATETIME values.
>
>      modified:
>        mysql-test/r/type_datetime.result
>        mysql-test/t/type_datetime.test
>        sql/item.cc
> === modified file 'mysql-test/r/type_datetime.result'
> --- a/mysql-test/r/type_datetime.result	2010-09-07 06:45:00 +0000
> +++ b/mysql-test/r/type_datetime.result	2010-10-01 11:38:16 +0000
> @@ -680,5 +680,17 @@ id	select_type	table	type	possible_keys	
>   1	SIMPLE	t2	const	PRIMARY	PRIMARY	8	const	1	Using index
>   DROP TABLE t1,t2;
>   #
> +# Bug#57095: Wrongly chosen expression cache type led to a wrong
> +#            result.
> +#
> +CREATE TABLE t1 (`b` datetime );
> +INSERT INTO t1 VALUES ('2010-01-01 00:00:00'), ('2010-01-01 00:00:00');
> +SELECT * FROM t1 WHERE b<= coalesce(NULL, now());
> +b
> +2010-01-01 00:00:00
> +2010-01-01 00:00:00
> +DROP TABLE t1;
> +#
> +#
>   # End of 5.5 tests
>   #
>
> === modified file 'mysql-test/t/type_datetime.test'
> --- a/mysql-test/t/type_datetime.test	2010-09-07 06:45:00 +0000
> +++ b/mysql-test/t/type_datetime.test	2010-10-01 11:38:16 +0000
> @@ -485,5 +485,15 @@ explain select * from t2 where f1=STR_TO
>   DROP TABLE t1,t2;
>
>   --echo #
> +--echo # Bug#57095: Wrongly chosen expression cache type led to a wrong
> +--echo #            result.
> +--echo #
> +CREATE TABLE t1 (`b` datetime );
> +INSERT INTO t1 VALUES ('2010-01-01 00:00:00'), ('2010-01-01 00:00:00');
> +SELECT * FROM t1 WHERE b<= coalesce(NULL, now());
> +DROP TABLE t1;
> +--echo #
> +
> +--echo #

Since this, according to the bug report, only happens with InnoDB, I 
think we should hard code the engine for this example.

>   --echo # End of 5.5 tests
>   --echo #
>
> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2010-09-24 13:18:45 +0000
> +++ b/sql/item.cc	2010-10-01 11:38:16 +0000
> @@ -7356,9 +7356,11 @@ Item_cache* Item_cache::get_cache(const
>     case DECIMAL_RESULT:
>       return new Item_cache_decimal();
>     case STRING_RESULT:
> -    if (item->field_type() == MYSQL_TYPE_DATE ||
> -        item->field_type() == MYSQL_TYPE_DATETIME ||
> -        item->field_type() == MYSQL_TYPE_TIME)
> +    /* Not all functions that return DATE/TIME are actually DATE/TIME funcs. */
> +    if ((item->field_type() == MYSQL_TYPE_DATE ||
> +         item->field_type() == MYSQL_TYPE_DATETIME ||
> +         item->field_type() == MYSQL_TYPE_TIME)&&
> +        (const_cast<Item*>(item))->result_as_longlong())
>         return new Item_cache_datetime(item->field_type());
>       return new Item_cache_str(item);

If you can use Item_cache_str for TYPE_DATETIME when result is not 
available as longlong, why could you not just convert the longlong to a 
string in other cases and still use Item_cache_str?  What is really the 
purpose of Item_cache_datetime?

--
Øystein


>     case ROW_RESULT:
>
>
>
>
>




Thread
bzr commit into mysql-5.5-bugteam branch (epotemkin:3213) Bug#57095Evgeny Potemkin1 Oct
  • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3213) Bug#57095Øystein Grøvlen6 Oct
    • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3213) Bug#57095Evgeny Potemkin7 Oct
      • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3213) Bug#57095Roy Lyseng7 Oct
      • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3213) Bug#57095Øystein Grøvlen7 Oct