Hi Sergei! On Mon, Jan 17, 2022 at 7:56 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jan 13, Aleksey Midenkov wrote:
But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena.
stmt_arena cannot be used as long as 'expr' item is created on expr_arena. 'expr' item is created at the vcol expression parse and is reused during the whole TABLE lifetime. If some of the containee items later were allocated on stmt_arena, 'expr' item will try to access them in a different statement when they are already released. That is what this bugfix tries to address.
Yes, this fix introduces a small and in most cases imperceptible memory leak which can be easily overcame by FLUSH TABLES. The fix for this leak is non-trivial.
1. vcol expr is not always refixed, but conditionally controlled by vcols_need_refixing. And we must remove this control and truly refix always.
2. we must clone 'expr' item into statement mem_root at each statement.
I have attached the leak fix PoC into this email. I do not know whether this is worth it.
vcols_need_refixing isn't run-time conditional, it depends on vcol flags, a specific vcol always needs refixing or never does. And a table has vcols_need_refixing if any of the vcols nees it.
Thus, I think, a simpler fix would be to split the cleanup and fix_fields. Now both are done in fix_session_vcol_expr().
We can do cleanup at the end of every statement and fix_fields at the beginning of a statement. Just like for normal non-persistent items. For every vcol that needs it. This way they can safely use execution arena.
Nice catch! Done. Btw, expr_arena was already used in update_virtual_fields(), update_default_fields(), set_default(). So the leak is already there and now we just didn't make it bigger. I marked leak places with TODO comments.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok