← Back to team overview

maria-developers team mailing list archive

Re: 06942363ebc: MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable


Hi Sergei,

Thanks for your review. Comments follow:

On 6/7/20 9:59 PM, Sergei Golubchik wrote:
Hi, Alexander!

On Jun 07, Alexander Barkov wrote:
revision-id: 06942363ebc (mariadb-10.5.2-311-g06942363ebc)
parent(s): 0d6d63e1505
author: Alexander Barkov <bar@xxxxxxxxxxx>
committer: Alexander Barkov <bar@xxxxxxxxxxx>
timestamp: 2020-06-03 12:11:07 +0400

MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable

diff --git a/sql/item.cc b/sql/item.cc
index 8ea6366e6c4..5800b9bc95e 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -764,6 +764,29 @@ bool Item_field::collect_item_field_processor(void *arg)
+void Item_ident::undeclared_spvar_error() const

Currently it's used only in Item_field::unknown_splocal_processor().
Do you plan to use this somewhere else?
Why is it a separate method and in Item_ident?

It's also used in Item_ref::unknown_splocal_processor().

Item_ident their common parent.

+  /*
+    We assume this is an unknown SP variable, possibly a ROW variable.
+    Print the leftmost name in the error:
+      SET var=a;      -> a
+      SET var=a.b;    -> a
+      SET var=a.b.c;  -> a
+  */
+  my_error(ER_SP_UNDECLARED_VAR, MYF(0),  db_name.str    ? db_name.str :
+                                          table_name.str ? table_name.str :
+                                          field_name.str);
+bool Item_field::unknown_splocal_processor(void *arg)
+  DBUG_ENTER("Item_field::unknown_splocal_processor");
+  undeclared_spvar_error();
+  DBUG_RETURN(true);
  bool Item_field::add_field_to_set_processor(void *arg)
@@ -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);

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?

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:

  if (processor == &Item::unknown_splocal_processor)

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?

But returning to Item_ref, I just realized that this statement
(without tables) incorrectly reports the unknown variable error:


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:
- The intent of this MDEV was to catch simple obvious cases, like:
  SET x=long_variable_name_with_a_typo;
- Having is not very  useful when there are not tables.
  It's very unlikely that someone will use it,
  moreover will make a typo.
- We don't have to touch Item_ref::walk() if we skip HAVING, it seems.
  Or do we still have to?


+  if (ref && *ref)
+    return (*ref)->walk(processor, walk_subquery, arg) ||
+           (this->*processor)(arg);
+  return false;
    Transform an Item_ref object with a transformer callback function.
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx

Follow ups