Hi, Alexander!
On Mar 06, Alexander Barkov wrote:
> >> @@ -3378,16 +3379,16 @@ String *Item_func_conv_charset::val_str(String
> >> *str)
> >> DBUG_ASSERT(fixed == 1);
> >> if (use_cached_value)
> >> return null_value ? 0 : &str_value;
> >> - String *arg= args[0]->val_str(str);
> >> + String *arg= args[0]->val_str(&tmp_value);
> >> uint dummy_errors;
> >> if (args[0]->null_value)
> >> {
> >> null_value=1;
> >> return 0;
> >> }
> >> - null_value= tmp_value.copy(arg->ptr(), arg->length(), arg->charset(),
> >> - conv_charset, &dummy_errors);
> >> - return null_value ? 0 : check_well_formed_result(&tmp_value);
> >> + null_value= str->copy(arg->ptr(), arg->length(), arg->charset(),
> >> + conv_charset, &dummy_errors);
> >> + return null_value ? 0 : check_well_formed_result(str);
> >> }
> >
> > That's a good idea. I'd expect it to fix the bug for both queries, with
> > and without SUBSTR.
>
> I tested this change alone, without adding the "res->is_alloced()" part.
> It fixes both queries, with and without SUBSTR.
Great!
> > But (sorry) you're fixing only Item_func_conv_charset. Is it the
> > only Item that stores the result in its internal buffer (tmp_value
> > or str_value)? Any other item that does it will cause the same bug.
>
> There are more Items:
>
> - around 16 doing "return &tmp_value;"
> - around 4 doing "return &str_value;"
>
> Here's an example of another bad result:
>
> DROP TABLE IF EXISTS t1;
> CREATE TABLE t1 (t VARCHAR(10) CHARSET latin1);
> INSERT INTO t1 VALUES('1234567');
> SELECT CONCAT(t2,'-',t2) c2 FROM (SELECT REVERSE(t) t2 FROM t1) sub;
>
> +----------------+
> | c2 |
> +----------------+
> | 76543217654321 |
> +----------------+
>
> It should be '7654321-7654321' instead.
>
> Should we fix all of them under terms of MDEV-10306?
I'd say yes. What do you think?
And, by the way, could you also fix the comment in item.h around
Item::val_str(String *) not to say that it can only change allocated
strings? May be it should say about const (String::make_const) strings
instead. Thanks. Also you could add something about this your CONCAT
fix. Like, the item can use internal buffer as a temporary storage,
but the result should be in the String argument, not the other way
around. Ideally, we'd have some way to enforce it, but I couldn't think
of anything.
Regards,
Sergei
Chief Architect MariaDB
and secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
