maria-developers team mailing list archive

[jerome brauge <j.brauge@xxxxxxxxxxx>]

Hello Alexander,
Thanks for your great review.

Regards,
Jérôme.

> -----Message d'origine-----
> De : Alexander Barkov [mailto:bar@xxxxxxxxxxx]
> Envoyé : dimanche 5 février 2017 14:45
> À : jerome brauge; MariaDB Developers (maria-
> developers@xxxxxxxxxxxxxxxxxxx)
> Objet : Re: MDEV-10697 - bb-10.2-compatibility
> 
> Hello Jerome!
> 
> On 02/03/2017 09:46 PM, jerome brauge wrote:
> > Hello,
> >
> > I've wrote a patch for MDEV-10697 (goto statement for sql_mode=oracle).
> >
> > If someone can review it.
> >
> > Thanks.
> >
> > Best regards,
> >
> > Jérôme.
> >
> 
> 
> Thank you very much for working on this!
> You've done a great job! Tests looks very good, and you have taken care of
> the most difficult part - GOTOs crossing nested blocks with cursors and
> exceptions, the code makes sure that cpop/hpop are correctly done.
> Excellent!
> 
> Note, I haven't reviewed the entire patch thoroughly yet.
> Will do it in the beginning of the next week.
> I have suggestions and questions related to sql_yacc_ora.yy at this point.
> 
> 
> - Can you please clone the latest bb-10.2-compatibility and adjust the patch?
>   It's now different. For example, there is no such thing like
>   sp_unlabeled_block_not_atomic any more, because MDEV-10655 has been
> pushed.
>

Done.

> - Can't we remove sp_declare_label_alone
>   and use label_declaration_oracle directly?

Done.
 
> - Please rename sp_labeled_stmt to sp_labelable_stmt.
>   "sp_labeled_stmt" misleadingly makes the reader think that this is
>   a statement that HAS a label. But in fact, this is a statement
>   that CAN have a label (but not necessarily has).
> 
>   The same for sp_unlabeled_stmt: please rename it to
> sp_non_labelable_stmt.
>   As the grammar implements "a statement that CANNOT have a label"
>   rather than"a statement that does not have a label".
>   By the way, perhaps there is no a need for this rule at all.
>   It's used only one time. So its sub-rules can be moved
>   directly to sp_proc_stmt.

Done (I agree, names was very bad...)

> - Why there is a need for two separate label lists
>   sp_pcontext::m_labels and sp_pcontext::jump labels?
>   Can't we put all labels the same list?
>   Does the fact that LOOP labels and GOTO labels reside
>   in different lists mean that this won't compile:
> 
> DROP PROCEDURE p1;
> CREATE PROCEDURE p1 AS
>   a INT := 1;
> BEGIN
>   <<lab>>
>   FOR i IN a..10 LOOP
>     IF i = 5 THEN
>       a:= a+1;
>       GOTO lab;
>     END IF;
>   END LOOP;
> END;
> /
> 
> The above works fine in Oracle.
> So we should try to support this.

In fact your example already works.

At the beginning, I was afraid of breaking your logic (push/pop of label out of order).
But your example illustrate very well the need of two list : label "lab" has two meanings here
 - GOTO lab : must go before the beginning of the loop
 - CONTINUE lab : must go to the beginning of the loop

I add these tests to the suite.

> - I did not find tests for labels and GOTO inside EXCEPTION blocks.
>   Can you please check if it works and add tests?

Done.

> - Please don't use dynamic_cast. We build without RTTI,
>   so it won't compile on some platforms.

Done.

> - Please stay under the limit of 80 characters per line.
>   Some lines in the patch are longer.

Done.

> - Please rename func_goto.test to sp-goto.test.
>   We use the func_ prefix for tests covering
>   Item_func descendants.

Done.
 
> - What is the purpose of sp_proc_stmts1_implicit_block?
>   Why can't we use sp_proc_stmts1 directly?

I need to have a new sp_pcontext even if I haven't an explicit bloc (begin - end) to isolate the scope of label definition.
It's the case for IF/ELSE, CASE/WHEN and probably for EXCEPTION/WHEN.
At this time we must add a begin/end in the when clause of exception handler if we have several instruction. 
CREATE or replace procedure p1()
AS
  a INT;
BEGIN
  begin
    SELECT a INTO a FROM information_schema.tables;
  EXCEPTION
    WHEN TOO_MANY_ROWS THEN
           a:=10;
           goto lab;
    WHEN NO_DATA_FOUND THEN
           a:=-5;
  end;
  <<lab>>
  return ;
END;

The above works fine in Oracle.

> Thanks!

Attachment: 0001-MDEV-10697-sql_mode-ORACLE-GOTO-statement.patch
Description: 0001-MDEV-10697-sql_mode-ORACLE-GOTO-statement.patch