Hi, Jacob! On Apr 06, Jacob Mathew wrote:
Hi Sergei,
After doing more testing with the recursive call to Item::cleanup_processor(), I have found problems with column default values that reference another column as an argument to a date/time function. At the point when fix_session_vcol_expr() is called, Item::check_vcol_func_processor() has already been called recursively and Item_field::check_vcol_func_processor() has set Name_resolution_context *context= 0. This causes a crash when the recursive call to Item::cleanup_processor() accesses members of context.
Exactly. There is no need to run fix_fields for actual fields, for Item_field. See, I've written in the review - vcol->expr->cleanup(); + vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0); Note there is no "cleanup_excluding_fields_processor" in the code. There is cleanup_processor(), and it doesn't work because it tries to re-fix fields. And there is cleanup_excluding_const_fields_processor(), it doesn't work either, because it only excludes const fields, and still re-fixes other fields. That's why I wrote that you need "cleanup_excluding_const_fields_processor" which is a processor that will exclude *all* fields, const or not.
I think I will need to spend a lot more time learning this area to get this working. So, I am currently leaning toward using a separate recursive call to Item::clean_cached_value() in fix_session_vcol_expr() and leaving its non-recursive call to cleanup() as is.
But the cleanup must be recursive! Unrelated to cached items, I had it in my review - there is a test case with no cached items, that shows a bug with non-recursive cleanup. Basically the result is correct for a virtual column that uses current_user(), but not correct when the virtual column uses concat(current_user()).
Is there a reason why the Virtual_column_info class doesn't include the vcol type? Is there a reason why the Virtual_column_info class shouldn't include the vcol type?
I think it could include the type. At least I cannot think of any reason why it shouldn't. Regards, Sergei Chief Architect MariaDB and security@mariadb.org