Hi, Alexander! On Jun 07, Alexander Barkov wrote:
@@ -8012,6 +8035,27 @@ void Item_ref::cleanup()
+bool Item_ref::unknown_splocal_processor(void *arg) +{ + DBUG_ENTER("Item_ref::unknown_splocal_processor"); + DBUG_ASSERT(type() == REF_ITEM); + DBUG_ASSERT(ref_type() == REF); + undeclared_spvar_error(); + DBUG_RETURN(true); +} + +bool Item_ref::walk(Item_processor processor, bool walk_subquery, void *arg) +{ + if (processor == &Item::unknown_splocal_processor) + return unknown_splocal_processor(arg); + if (ref && *ref) + return (*ref)->walk(processor, walk_subquery, arg) || + (this->*processor)(arg); + return false; +}
Why is that? This is very unusual, normally walk() should not change its behavior based on the processor.
Note, ref is NULL when walk(Item::unknown_splocal_processor) is called. We have to change Item_ref::walk() somehow: It does not call (this->*processor)(arg) when ref is NULL. Do you have ideas how to do it in a better way?
Let's change it to call this->*processor unconditionally. And fix individual processors to check for ref!=NULL. It looks like the only processor that will need it is bool enumerate_field_refs_processor(void *arg) - { return (*ref)->enumerate_field_refs_processor(arg); } + { return ref && (*ref)->enumerate_field_refs_processor(arg); }
Btw, your proposed solution with calling walk(walk_subquery=false) and with doing recursion in Item_subselect::unknown_splocal_processor() also changes behavior: calling walk() from a processor does not look "normal" :)
This solution effectively implements exactly the same logic:
Item_subselect::walk() { ... if (processor == &Item::unknown_splocal_processor) do_recursion_not_normally(); ... }
just in a less obvious way.
I think I'd prefer it to be written the same way ^^^ inside Item_subselect::walk(), like I did in Item_ref, rather than calling the recursion from the processor.
A possible option would be to change the parameter "bool walk_subquery" from bool to something else, so the caller can pass more options. What do you think?
I think I prefer what I suggested, to call walk from the processor :) walk() method defines the general API for traversing the tree. An individual processor is the user of that API. If some user has special weird needs, I'd prefer that to be implemented in that user processor, not in the general API used by many.
But returning to Item_ref, I just realized that this statement (without tables) incorrectly reports the unknown variable error:
SET a=(SELECT 1 AS x HAVING x);
I can see two ways:
1. Skip HAVING for now - don't try to catch unknown variables in HAVING.
2. For every Item_ident in HAVING go through the select list and check aliases.
I'm inclined to 1:
Agree Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org