Hi Sergei! On Thu, Jan 6, 2022 at 7:13 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
Could you please explain in the commit comment what the bug was and what you are fixing?
I have added a new message to the last commit. This will be the message of the squashed patch.
I wasn't able to understad that, so I had to debug those crashes myself.
So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different.
Yes.
I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere.
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.
Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over?
That was the alias fix. Refix of Item_field was required. I returned back cleanup_excluding_fields_processor() and removed VCOL_TABLE_ALIAS processing despite my opinion to apply that.
We've already discussed VCOL_TABLE_ALIAS, no need to go there again.
On Jan 06, Aleksey Midenkov wrote:
revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) parent(s): a565e63c54c author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2021-12-29 00:46:31 +0300 message:
MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
When table is reopened from cache vcol_info contains stale expression. We refresh expression via TABLE::vcol_fix_exprs() but first we must prepare a proper context (Vcol_expr_context) and do some preparations which meet some requirements:
1. fields must not be fixed, we unfix them with cleanup() via cleanup_processor.
2. Item_ident must have cached_table cleared. Again, this is stale data which was already freed. cached_table interferes with find_field_in_tables().
We cannot clear cached_table directly in Item_ident::cleanup() method. Lots of spaghetti logic depends on cached_table existence even after cleanup() done.
3. If Item_ident has table_name non-NULL we trigger expr update. That is done via Item_ident::check_vcol_func_processor() and VCOL_TABLE_ALIAS flag.
(4-7) The below conditions are prepared by Vcol_expr_context and used in both fix_session_expr_for_read() and TABLE::vcol_fix_exprs():
4. Any fix_expr() must be done on expr_arena as there may be new items created. It was a bug in fix_session_expr_for_read(). It was just not reproduced because there were no second refix. Now refix is done for more cases so it does reproduce. Tests affected: vcol.binlog
5. Also name resolution context must be narrowed to the single table with all crutches in place. Tests affected: vcol.update
6. sql_mode must be clean and not fail expr update.
sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc must not affect vcol expression update. If the table was created successfully any further evaluation must not fail. Tests affected: main.func_like
7. Warnings are disabled during expr update. It is assumed that these warnings were printed before during initialization phase or later during execution phase. Tests affected: vcol.wrong_arena
Dictionary: refresh, update and refix are the synonyms for vcol_fix_exprs() or fix_session_expr_for_read() calls.
8. cleanup_excluding_fields_processor() removed. It was just a quick hack to exclude wrongly working Item_field from processing. Now it works due to correct execution environment (Vcol_expr_context). Related to MDEV-10355.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok