maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Dec 21, Alexander Barkov wrote:
> >> 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.

okay

> Please suggest a good commit comment.

Just explain all the above there.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx