maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 05/05/2016 11:27 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> Sure, that's ok. A couple of comments below:
>
> On May 05, Alexander Barkov wrote:
> > diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> > index a6bbfc8..0d8bfb3 100644
> > --- a/sql/sql_select.cc
> > +++ b/sql/sql_select.cc
> > @@ -988,6 +988,7 @@ JOIN::prepare(TABLE_LIST *tables_init,
> >       }
> >       if (thd->lex->derived_tables)
> >       {
> > +      // SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE()
>
> What does that mean? An example of a query that is not covered by your
> current refactoring and still needs this check?

Yes. I added this comment:

    if (thd->lex->derived_tables)
    {
      /*
        Queries with derived tables and PROCEDURE are not allowed.
        Many of such queries are disallowed grammatically, but there
        are still some complex cases:
          SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE()
      */
      my_error(ER_WRONG_USAGE, MYF(0), "PROCEDURE",
               thd->lex->derived_tables & DERIVED_VIEW ?
               "view" : "subquery");
      goto err;
    }


By the way, fixing this particular restriction grammatically might
need huge refactoring in table_ref and friends.
We'll see later, if it's really feasible.


> >         my_error(ER_WRONG_USAGE, MYF(0), "PROCEDURE",
> >                  thd->lex->derived_tables & DERIVED_VIEW ?
> >                  "view" : "subquery");
> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index 5e1f5dc..445f986 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -5695,7 +5695,10 @@ create_select:
> >             {
> >               Select->parsing_place= NO_MATTER;
> >             }
> > -          table_expression
> > +          opt_table_expression
> > +          opt_order_clause
> > +          opt_limit_clause
> > +          opt_select_lock_type
> >             {
>
> Looks like you might want to create a rule for that:
>
>    opt_xxx: opt_table_expression opt_order_clause opt_limit_clause
>             opt_select_lock_type;
>
> because it seems to be used many times in different parts of the grammar

I tried to do it but failed with new shift/reduce conflicts, which I was
not able to find quickly a reason for.

Now I think I'll just clean-up the other things first and then try to
group these things again later. I added a remainder:

          /*
            TODO:
            The following sequence repeats a few times:
                  opt_table_expression
                  opt_order_clause
                  opt_limit_clause
                  opt_select_lock_type
            Perhaps they can be grouped into a dedicated rule.
          */
          opt_table_expression
          opt_order_clause
          opt_limit_clause
          opt_select_lock_type




Btw, in the MDEV-8909 branch you also had this block four times.
Perhaps for some similar reason :)


> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx