Hi Sergey, Martin,
Sergey Glukhov wrote:
> Hi Martin,
>
> On 12/28/2010 11:33 AM, Martin Hansson wrote:
>> Hi Sergey,
>>
>> Sergey Glukhov skrev 2010-12-23 12.44:
>>> Hi Martin, see comments below
>>>
>>> On 12/22/2010 03:30 PM, Martin Hansson wrote:
>>>> #At file:///data0/martin/bzrroot/bug58165/5.1bt-lazy_copy/ based on
>>>> revid:sergey.glukhov@stripped
>>>>
>>>>
>>>> modified:
>>>> mysql-test/r/func_str.result
>>>> mysql-test/t/func_str.test
>>>> sql/item_strfunc.cc
>>>> sql/sql_string.cc
>>> ...
>>>> === modified file 'sql/item_strfunc.cc'
>>>> --- a/sql/item_strfunc.cc 2010-11-11 10:25:23 +0000
>>>> +++ b/sql/item_strfunc.cc 2010-12-22 12:30:13 +0000
>>>> @@ -1305,8 +1305,15 @@ String *Item_func_substr_index::val_str(
>>>> null_value=0;
>>>> uint delimiter_length= delimiter->length();
>>>> if (!res->length() || !delimiter_length || !count)
>>>> - return&my_empty_string; // Wrong parameters
>>>> -
>>>> + {
>>>> + if (str->copy("", 0, default_charset_info)) // Wrong
>>>> parameters
>>>> + {
>>>> + /* Out of memory */
>>>> + null_value= true;
>>>> + return NULL;
>>>> + }
>>>> + return str;
>>>> + }
>>>> res->set_charset(collation.collation);
>>>>
>>>> #ifdef USE_MB
>>>>
>>>
>>> Using my_empty_string is not safe, the bug shows it.
>>> So we should remove my_empty_string from all places
>>> where it is used.
>> Removing my_empty_string altogether is way too large to be done as an
>> MRU bug fix (Believe me, I tried.) I suggest filing a separate report
>> about it. Let's make this customer happy first and then we'll deal
>> with the other problems we discovered while working on it.
> I was too optimistic with the request about removing my_empty_string
> from all places. As far as I can see problematic places are
> set_var.cc, sql_class.cc. However we could fix all places in
> item_strfunc.cc at least. it should be not a problem.
> It seems we should introduce new method, say
>
> Item::empty_string()
> {
> str_value.set("", 0, collation.collation);
> return &str_value;
> }
Sergey, I like your idea about introducing this new method.
Please check if it can be moved to Item_str_func:
String *Item_str_func::empty_string()
{
str_value.set("", 0, collation.collation);
return &str_value;
}
Perhaps it could also be named "str_value_empty",
to accent that we're returning "str_value".
>
> and replace &my_empty_string with empty_string().
>
It should be safe to replace all &my_empty_string instances
to the new method calls in item_str_func.cc,
even under terms of a MRU bug. Looks like a fairly small change:
"grep my_empty_string item_strfunc.cc" reports about 22 lines.
Note, my_empty_string is a reminder from 4.0 times,
when we had a single global character set for the entire server.
my_empty_string should be eventually removed everywhere from the
sources. Fixing item_strfunc.cc looks like a good start.
>
> PS Bar, I would like to know your opinion about this...
>
> Regards,
> Gluh
>
