maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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
> > message:
> >
> > 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");
> > +  DBUG_ASSERT(type() == FIELD_ITEM);
> > +  undeclared_spvar_error();
> > +  DBUG_RETURN(true);
> > +}
> > +
> > +
> >   bool Item_field::add_field_to_set_processor(void *arg)
> >   {
> >     DBUG_ENTER("Item_field::add_field_to_set_processor");
> > @@ -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:

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?



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:
- 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?


Thanks.

> > +  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.
> >   
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx