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

I am satisfied with your answers. Approved!

--
Øystein


On 10/07/2010 08:29 AM, Evgeny Potemkin wrote:
> 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