maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #10362
Re: MDEV-10697 - bb-10.2-compatibility
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
Follow ups
References