maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Jerome,

On 02/06/2017 08:44 PM, jerome brauge wrote:
> Hello Alexander,
> Thanks for your great review.

Thanks for addressing my suggestions.
Please see my comments inline, and some more things in the bottom of the
letter:

> 
> 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...)


Sorry, I might be not clear enough.

Please rename "sp_non_labelable_stmt" to "sp_labelable_stmt".
My idea of this name was that these statements are ALLOWED to
have a label, they can appear either with a label or without a label.

Btw, are there any statements in PL/SQL that cannot have a label?


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

Thanks for the explanation. Sounds reasonable to have separate lists.
Can you please add a comment near m_jump_labels?
Something like this:

/*
  In the below example the label <<lab>> has two meanings:
  - GOTO lab : must go before the beginning of the loop
  - CONTINUE lab : must go to the beginning of the loop
  We solve this by storing block labels and goto labels into separate lists.

  BEGIN
    <<lab>>
    FOR i IN a..10 LOOP
      ...
      GOTO lab;
      ...
      CONTINUE lab;
      ...
    END LOOP;
  END;
*/


> I add these tests to the suite.

Excellent.

> 
>> - 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.

There is still one dynamic_cast left in sp_head.cc.
sp_head::check_unresolved_jump().

> 
> 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. 

Thanks for clarification!

> 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!
> 

Here are some more suggestions/questions:


- It seems the patch has some redundant trailing spaces
  and new lines. Please do "git diff --check" before doing
  "git commit".

- Please use two spaces for indentation.
  In some cases you use four and three spaces, e.g. in
  sp_head::push_backpatch_jump(), sp_head::check_unresolved_jump(),
  LEX::sp_goto_statement() (before "delayedlabel= lab;")

- In conditions we try not to use redundant parentheses:

    if ((m_scope == HANDLER_SCOPE)&&(m_parent))

  We usually write like this (it's easier to read):
    if (m_scope == HANDLER_SCOPE && m_parent)

- Please use static_cast, e.g.:
    static_cast<sp_inst_cpop*>(bp->instr)
  instead of C-style cast"
    ((sp_instr_cpop *)(bp->instr))

  static_cast is safer. In case of possible future refactoring in the
  class hierarchy, static_cast will catch pointer incompatibility at
  compilation time, while C-style cast will just silently fallback to
  reinterpret_cast.


- It seems sp_head::check_unresolved_jump() should return a bool result:
  true on failure and false on success, and the calling code in
  sql_yacc_ora.yy should abort if it fails:
    if (sp->check_unresolved_jump())
      MYSQL_YYABORT;

- I suggest to rename "jump" to "goto" in all new members and methods,
  e.g. m_jump_labels, backpatch_jump(), check_unresolved_jump(),
  find_jump_label(), JUMP in the enum backpatch_instr_type,
  and so on.

- In tests please use error names rather than numeric values,
  e.g. "--error ER_SP_LILABEL_MISMATCH" instead of "--error 1308".
  See mysqld_ername.h for name definitions.


- The grammar does not allow labels before labels.
  The below examples return a syntax error:

DROP PROCEDURE p1;
DELIMITER $$
CREATE PROCEDURE p1 AS
BEGIN
  <<lab1>>
  <<lab2>>
  NULL;
END;
$$
DELIMITER ;


DROP PROCEDURE p1;
DELIMITER $$
CREATE PROCEDURE p1 AS
BEGIN
  <<lab1>>
  <<lab2>>
  BEGIN
    NULL;
  END;
END;
$$
DELIMITER ;


DROP PROCEDURE p1;
DELIMITER $$
CREATE PROCEDURE p1 AS
BEGIN
  <<lab1>>
  <<lab2>>
  FOR i IN 1..10 LOOP
    NULL;
  END LOOP;
END;
$$
DELIMITER ;

  Similar procedures work fine in Oracle.

Thanks!