[Maria-developers] MDEV-13790 UNHEX() of a somewhat complicated CONCAT() returns NULL
Hello Sergei, Please review a patch for MDEV-13790. It removes the attempt to search the best suitable buffer and just returns the result in "str" passed to val_str(). I'm quite sure this optimization gives nothing. In can be useful only under very rare conditions, but generates so many bugs. Moreover, in 10.3 (or 10.4) we'll work on reentrant items. So all Item_xxx::tmp_buffer will be gone anyway, together with this optimization. I think an optimization proposal. Why not to introduce a new method: class Item { public: .. virtual append_to_string(String *str); .. } So for example Field_varchar could write directly to "str" passed to Item_func_concat::val_str(). This will make sure that the first allocation is done on the target "str", and "str" will always be the best buffer instead of any Item_xxx::tmp_value. Thanks!
Hi, Alexander! On Nov 03, Alexander Barkov wrote:
Hello Sergei,
Please review a patch for MDEV-13790.
It removes the attempt to search the best suitable buffer and just returns the result in "str" passed to val_str().
I'm quite sure this optimization gives nothing. In can be useful only under very rare conditions, but generates so many bugs.
There're bunch of tricks. What do they all do, could you list them? One handles the case when an argument is a substring of the already concatenated string - this is very weird and rare, I agree. Other accumulates the result in the first argument's result - this avoids one memcpy per row. Doesn't sound like much, but it's not rare, so should be very easy to benchmark. Yet another doubles the buffer size every time - this might be useful to keep, it easily fits in your new code. What are other tricks that concat was using? Btw, one comment about your patch — please don't call current_thd in a loop. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, Thanks for review. Comments follow inline: On 01/28/2018 07:22 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Nov 03, Alexander Barkov wrote:
Hello Sergei,
Please review a patch for MDEV-13790.
It removes the attempt to search the best suitable buffer and just returns the result in "str" passed to val_str().
I'm quite sure this optimization gives nothing. In can be useful only under very rare conditions, but generates so many bugs.
There're bunch of tricks. What do they all do, could you list them?
One handles the case when an argument is a substring of the already concatenated string - this is very weird and rare, I agree.
Other accumulates the result in the first argument's result - this avoids one memcpy per row. Doesn't sound like much, but it's not rare, so should be very easy to benchmark.
In my patch the first argument goes directly to "str" parameter of Item_func_concat::val_str(). There should not be an extra memcpy here.
Yet another doubles the buffer size every time - this might be useful to keep, it easily fits in your new code.
As discussed on IRC, I kept this code. Please find a new version attached.
What are other tricks that concat was using?
Btw, one comment about your patch — please don't call current_thd in a loop.
Fixed.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik