maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hello Sergei,


On 12/21/2016 12:48 AM, Sergei Golubchik wrote:
> Hi, Alexander!
> 
> On Dec 19, Alexander Barkov wrote:
>> Hello Sergei,
>>
>> Please review a patch for MDEV-10386.
>>
>> Thanks!
> 
>> diff --git a/sql/item.cc b/sql/item.cc
>> index 53666aa..44bd620 100644
>> --- a/sql/item.cc
>> +++ b/sql/item.cc
>> @@ -1164,7 +1164,9 @@ Item *Item_cache::safe_charset_converter(CHARSET_INFO *tocs)
>>    if (conv == example)
>>      return this;
>>    Item_cache *cache;
>> -  if (!conv || !(cache= new Item_cache_str(conv)))
>> +  if (!conv ||
>> +      conv->fix_fields(current_thd, (Item **) NULL) ||
>> +      !(cache= new Item_cache_str(conv)))
>>      return NULL; // Safe conversion is not possible, or OEM
>>    cache->setup(conv);
>>    cache->fixed= false; // Make Item::fix_fields() happy
> 
> Difficult to review without a commit comment.
> What's going on here? How did it work before without fix_fields?

I guess that the problem sneaked in with this commit:
b96c196f1cd5d77e524cbf952539bdd33c65ffc1

I didn't try to revert it though.
But it seems that this patch added a new call for
safe_charset_converter() without a corresponding fix_fields().
My patch eliminates this disbalance.

> Why is it suddenly needed?

Looks like a rarely used combination and it was not covered in mtr.
Btw, b96c196f1cd5d77e524cbf952539bdd33c65ffc1 doesn't have tests for
some reasons, neither has an MDEV.

Note, in case of literals it worked fine. Only subselects were affected.

Please suggest a good commit comment.

Thanks!

> 
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>