[Maria-developers] MDEV-10697 - bb-10.2-compatibility
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.
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. - Can't we remove sp_declare_label_alone and use label_declaration_oracle directly? - 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. - 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. - I did not find tests for labels and GOTO inside EXCEPTION blocks. Can you please check if it works and add tests? - Please don't use dynamic_cast. We build without RTTI, so it won't compile on some platforms. - Please stay under the limit of 80 characters per line. Some lines in the patch are longer. - Please rename func_goto.test to sp-goto.test. We use the func_ prefix for tests covering Item_func descendants. - What is the purpose of sp_proc_stmts1_implicit_block? Why can't we use sp_proc_stmts1 directly? Thanks!
Hello Alexander, Thanks for your great review. Regards, Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : dimanche 5 février 2017 14:45 À : jerome brauge; MariaDB Developers (maria- developers@lists.launchpad.net) 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!
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@mariadb.org] Envoyé : dimanche 5 février 2017 14:45 À : jerome brauge; MariaDB Developers (maria- developers@lists.launchpad.net) 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!
Hi Alexander, I'm sorry to waste your time for formatting and/or coding style issues. Is there a document that lists your best practices ? Regards, Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : mercredi 8 février 2017 07:06 À : jerome brauge; maria-developers Objet : Re: MDEV-10697 - bb-10.2-compatibility
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@mariadb.org] Envoyé : dimanche 5 février 2017 14:45 À : jerome brauge; MariaDB Developers (maria- developers@lists.launchpad.net) 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
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
sql_mode=oracle). 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?
Done. The only place where Oracle doesn't accept a label is just before an end of block. CREATE OR REPLACE PROCEDURE f20(res OUT VARCHAR) AS BEGIN res:=1; <<lab>> END; Failed with: PLS-00103: Encountered the symbol "END" when expecting one of the following: ( begin case declare exit for goto if loop mod null raise return select update while with <an identifier> <a double-quoted delimited-identifier> <a bind variable> << continue close current delete fetch lock insert open rollback savepoint set sql execute commit forall merge pipe purge We have the same behavior.
- 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;")
Done.
- 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.
Thanks for explanation. I'm a newbie in CPP (I know much more about pure C and java)
- 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;
Done (I had forgot a dynamic cast also)
- 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.
Done.
- 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.
Done.
- 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.
Done. Tests has been added to the suite.
Thanks!
Hello Jerome, The last version looks very good. Thanks!. Note, I haven't made a detailed review for sp-goto.test yet. Will reply about tests in a separate letter. On 02/08/2017 06:31 PM, jerome brauge wrote:
Hi Alexander, I'm sorry to waste your time for formatting and/or coding style issues. Is there a document that lists your best practices ?
No problem. Generally, we use MySQL coding style: https://dev.mysql.com/doc/internals/en/general-development-guidelines.html Please find coding style suggestions in the attached file. Also, please find comments inline: <cut>
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?
Done.
The only place where Oracle doesn't accept a label is just before an end of block.
Thanks. So my original assumption about two separate rules for labelable and non-labelable statements was wrong. All statements are labelable. I'm not an Oracle expert yet, but gradually learning it :) So we don't need sp_non_labelable_stmt them. Ok. <cut> Can you please also have a look into: https://mariadb.com/kb/en/mariadb/mariadb-contributor-agreement-frequently-a... We require contributors to share the code either under terms of MCA or BSD-new licenses. Also, when we finish reviews, would it be possible to ask you to make a git pull request? We prefer to merge pull requests instead of pushing the patch on behalf of the contributor. Thanks!
Hello Alexander, This is the latest version of the patch. I will try to do a pull request (I'm new on github) I send the agreement asap. Regard, Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 9 février 2017 13:16 À : jerome brauge Cc : maria-developers Objet : Re: MDEV-10697 - bb-10.2-compatibility
Hello Jerome,
The last version looks very good. Thanks!.
Note, I haven't made a detailed review for sp-goto.test yet. Will reply about tests in a separate letter.
On 02/08/2017 06:31 PM, jerome brauge wrote:
Hi Alexander, I'm sorry to waste your time for formatting and/or coding style issues. Is there a document that lists your best practices ?
No problem. Generally, we use MySQL coding style:
https://dev.mysql.com/doc/internals/en/general-development- guidelines.html
Please find coding style suggestions in the attached file.
Also, please find comments inline:
<cut>
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?
Done.
The only place where Oracle doesn't accept a label is just before an end of block.
Thanks. So my original assumption about two separate rules for labelable and non-labelable statements was wrong. All statements are labelable. I'm not an Oracle expert yet, but gradually learning it :)
So we don't need sp_non_labelable_stmt them. Ok.
<cut>
Can you please also have a look into:
https://mariadb.com/kb/en/mariadb/mariadb-contributor-agreement- frequently-asked-questions/
We require contributors to share the code either under terms of MCA or BSD-new licenses.
Also, when we finish reviews, would it be possible to ask you to make a git pull request?
We prefer to merge pull requests instead of pushing the patch on behalf of the contributor.
Thanks!
Hello Jerome. On 02/11/2017 11:27 AM, jerome brauge wrote:
Hello Alexander, This is the latest version of the patch.
The patch now looks very good for me.
I will try to do a pull request (I'm new on github) I send the agreement asap.
Please go ahead. Thanks!
Regard, Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 9 février 2017 13:16 À : jerome brauge Cc : maria-developers Objet : Re: MDEV-10697 - bb-10.2-compatibility
Hello Jerome,
The last version looks very good. Thanks!.
Note, I haven't made a detailed review for sp-goto.test yet. Will reply about tests in a separate letter.
On 02/08/2017 06:31 PM, jerome brauge wrote:
Hi Alexander, I'm sorry to waste your time for formatting and/or coding style issues. Is there a document that lists your best practices ?
No problem. Generally, we use MySQL coding style:
https://dev.mysql.com/doc/internals/en/general-development- guidelines.html
Please find coding style suggestions in the attached file.
Also, please find comments inline:
<cut>
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?
Done.
The only place where Oracle doesn't accept a label is just before an end of block.
Thanks. So my original assumption about two separate rules for labelable and non-labelable statements was wrong. All statements are labelable. I'm not an Oracle expert yet, but gradually learning it :)
So we don't need sp_non_labelable_stmt them. Ok.
<cut>
Can you please also have a look into:
https://mariadb.com/kb/en/mariadb/mariadb-contributor-agreement- frequently-asked-questions/
We require contributors to share the code either under terms of MCA or BSD-new licenses.
Also, when we finish reviews, would it be possible to ask you to make a git pull request?
We prefer to merge pull requests instead of pushing the patch on behalf of the contributor.
Thanks!
Hello Jerome. I merged you pull request to bb-10.2-compatibility. Thank you very much for this contribution! Greetings. On 02/11/2017 11:27 AM, jerome brauge wrote:
Hello Alexander, This is the latest version of the patch. I will try to do a pull request (I'm new on github) I send the agreement asap.
Regard, Jérôme.
-----Message d'origine----- De : Alexander Barkov [mailto:bar@mariadb.org] Envoyé : jeudi 9 février 2017 13:16 À : jerome brauge Cc : maria-developers Objet : Re: MDEV-10697 - bb-10.2-compatibility
Hello Jerome,
The last version looks very good. Thanks!.
Note, I haven't made a detailed review for sp-goto.test yet. Will reply about tests in a separate letter.
On 02/08/2017 06:31 PM, jerome brauge wrote:
Hi Alexander, I'm sorry to waste your time for formatting and/or coding style issues. Is there a document that lists your best practices ?
No problem. Generally, we use MySQL coding style:
https://dev.mysql.com/doc/internals/en/general-development- guidelines.html
Please find coding style suggestions in the attached file.
Also, please find comments inline:
<cut>
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?
Done.
The only place where Oracle doesn't accept a label is just before an end of block.
Thanks. So my original assumption about two separate rules for labelable and non-labelable statements was wrong. All statements are labelable. I'm not an Oracle expert yet, but gradually learning it :)
So we don't need sp_non_labelable_stmt them. Ok.
<cut>
Can you please also have a look into:
https://mariadb.com/kb/en/mariadb/mariadb-contributor-agreement- frequently-asked-questions/
We require contributors to share the code either under terms of MCA or BSD-new licenses.
Also, when we finish reviews, would it be possible to ask you to make a git pull request?
We prefer to merge pull requests instead of pushing the patch on behalf of the contributor.
Thanks!
participants (2)
-
Alexander Barkov -
jerome brauge