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

Thanks for the review:)

On 10/06/10 10:29, Øystein Grøvlen wrote:
> 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?
IMO this is the proper way to fix this bug.
>
> 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?
Well, it not very hard to make coalesce to return proper int representation for 
a datetime value. But the thing is that coalesce isn't a datetime function, it 
just returns first non-null value of its arguments. If we do this then to be 
consistent we have to add same feature to all string functions just in case 
they'll be processing datetime values. IMO this is wrong.

>
> Do we have the same issue with other functions? E.g. CASE?
The difference between coalesce and case is that case just passes val_xxx call 
to the appropriate item from the internal items array while coalesce is a 
descendant of Item_func_numhybrid and it operates in a bit different way.
Item_func_numhybrid::val_xxx first calls xxx_op function of appropriate result 
type (str_op in case of datetime values) and then converts what it got to asked 
type. So there is no easy and safe way to fix it this way, IMO.
With this fix even if there are similar problems they'll be hidden. Prior to it 
Item_cache_datetime expected a datetime returning item to also return correct 
int datetime representation. Now this is explicitly checked.
However it's worth adding better implementation of result_as_longlong for 
IF/CASE functions. Will do in the next patch.
>
>> 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.
As you can see expression caching doesn't depend on the engine. The reason why 
only Innodb is affected is the test case they provided. Myisam returns exact 
number of rows, thus optimizer pre-read all const tables and uses their fields 
as constants. Due to this the expression in their test case wasn't cached.
With two rows (like in this test case) both engines are affected.

>
>> --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?
Its purpose is to avoid unnecessary string -> (MYSQL_TIME|int) -> string 
conversions.
>
> --
> Øystein
>
>
>> case ROW_RESULT:
>>
>>
Regards, Evgen.
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