Re: 28deba0a611: MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
Hi, Alexander, Looks great! Just a couple of questions below On Nov 07, Alexander Barkov wrote:
revision-id: 28deba0a611 (mariadb-11.0.1-239-g28deba0a611) parent(s): e9573c05965 author: Alexander Barkov committer: Alexander Barkov timestamp: 2023-09-22 14:38:52 +0400 message:
MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
This patch adds PACKAGE support with SQL/PSM dialect for sql_mode=DEFAULT:
- CREATE PACKAGE - DROP PACKAGE - CREATE PACKAGE BODY - DROP PACKAGE BODY - Package function and procedure invocation from outside of the package: -- using two step identifiers SELECT pkg.f1(); CALL pkg.p1()
-- using three step identifiers SELECT db.pkg.f1(); CALL db.pkg.p1();
This is a non-standard MariaDB extension.
However, later this code can be used to implement the SQL Standard and DB2 dialects of CREATE MODULE.
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index af2ec678daa..fca39b63312 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1285,7 +1285,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); %left TRANSACTION_SYM TIMESTAMP PERIOD_SYM SYSTEM USER COMMENT_SYM
%left PREC_BELOW_SP_OBJECT_TYPE -%left FUNCTION_SYM +%left PACKAGE_MARIADB_SYM FUNCTION_SYM
Why PACKAGE_ORACLE_SYM and PACKAGE_ALLMODES_SYM don't need it? (same for BODY_ORACLE_SYM and BODY_ALLMODES_SYM)
/* @@ -3049,6 +3059,8 @@ sp_handler: | PROCEDURE_SYM { $$= &sp_handler_procedure; } | PACKAGE_ORACLE_SYM { $$= &sp_handler_package_spec; } | PACKAGE_ORACLE_SYM BODY_ORACLE_SYM { $$= &sp_handler_package_body; } + | PACKAGE_MARIADB_SYM { $$= &sp_handler_package_spec; } + | PACKAGE_MARIADB_SYM BODY_MARIADB_SYM { $$= &sp_handler_package_body; }
Why not to use here PACKAGE_ALLMODES_SYM and BODY_ALLMODES_SYM? (also, I'd personally call them simply PACKAGE_SYM and BODY_SYM)
;
@@ -19428,35 +19489,33 @@ create_routine: (Item_result) $8, $10)) MYSQL_YYABORT; } - | create_or_replace definer_opt PACKAGE_ORACLE_SYM + | create_or_replace definer_opt PACKAGE_ALLMODES_SYM opt_if_not_exists sp_name opt_create_package_chistics_init { sp_package *pkg; if (unlikely(!(pkg= Lex-> - create_package_start(thd, - SQLCOM_CREATE_PACKAGE, - &sp_handler_package_spec, - $5, $1 | $4)))) + create_package_start(thd, &sp_handler_package_spec, + $5, $1 | $4, + Lex->sp_chistics)))) MYSQL_YYABORT; pkg->set_c_chistics(Lex->sp_chistics);
Looks redundant
Lex->sphead->set_body_start(thd, YYLIP->get_cpp_tok_start()); } sp_tail_is opt_package_specification_element_list END - remember_end_opt opt_sp_name + remember_end_opt opt_trailing_sp_name { if (unlikely(Lex->create_package_finalize(thd, $5, $12, $11))) MYSQL_YYABORT; } - | create_or_replace definer_opt PACKAGE_ORACLE_SYM BODY_ORACLE_SYM + | create_or_replace definer_opt PACKAGE_ALLMODES_SYM BODY_ALLMODES_SYM opt_if_not_exists sp_name opt_create_package_chistics_init { sp_package *pkg; if (unlikely(!(pkg= Lex-> - create_package_start(thd, - SQLCOM_CREATE_PACKAGE_BODY, - &sp_handler_package_body, - $6, $1 | $5)))) + create_package_start(thd, &sp_handler_package_body, + $6, $1 | $5, + Lex->sp_chistics)))) MYSQL_YYABORT; pkg->set_c_chistics(Lex->sp_chistics);
same
Lex->sphead->set_body_start(thd, YYLIP->get_cpp_tok_start());
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei, Thanks for the review. The new patch is here: https://github.com/MariaDB/server/commit/a5344196e01239769bbf214ced020517800... It removes the two redundant calls: pkg->set_c_chistics(Lex->sp_chistics) Please find comments below. On 11/7/23 10:39 PM, Sergei Golubchik wrote:
Hi, Alexander,
Looks great! Just a couple of questions below
On Nov 07, Alexander Barkov wrote:
revision-id: 28deba0a611 (mariadb-11.0.1-239-g28deba0a611) parent(s): e9573c05965 author: Alexander Barkov committer: Alexander Barkov timestamp: 2023-09-22 14:38:52 +0400 message:
MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
This patch adds PACKAGE support with SQL/PSM dialect for sql_mode=DEFAULT:
- CREATE PACKAGE - DROP PACKAGE - CREATE PACKAGE BODY - DROP PACKAGE BODY - Package function and procedure invocation from outside of the package: -- using two step identifiers SELECT pkg.f1(); CALL pkg.p1()
-- using three step identifiers SELECT db.pkg.f1(); CALL db.pkg.p1();
This is a non-standard MariaDB extension.
However, later this code can be used to implement the SQL Standard and DB2 dialects of CREATE MODULE.
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index af2ec678daa..fca39b63312 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1285,7 +1285,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); %left TRANSACTION_SYM TIMESTAMP PERIOD_SYM SYSTEM USER COMMENT_SYM
%left PREC_BELOW_SP_OBJECT_TYPE -%left FUNCTION_SYM +%left PACKAGE_MARIADB_SYM FUNCTION_SYM
Why PACKAGE_ORACLE_SYM and PACKAGE_ALLMODES_SYM don't need it? (same for BODY_ORACLE_SYM and BODY_ALLMODES_SYM)
Keywords PACKAGE and BODY have different "reserved-ness" for sql_mode=DEFAULT and sql_mode=ORACLE. For sql_mode=ORACLE, they are more reserved and can act as identifiers only in the rule "reserved_keyword_udt_not_param_type". The grammar is non-conflicting, so "PACKAGE" does not need any tuning with precedence. For sql_mode=DEFAULT, for backward compatibility, they can act as identifiers in the rule "keyword_sp_var_and_label". So here we need to tell the parser that "PACKAGE" is an identifier, but only when it is not followed by "BODY". BODY itself does not need precedence tuning.
/* @@ -3049,6 +3059,8 @@ sp_handler: | PROCEDURE_SYM { $$= &sp_handler_procedure; } | PACKAGE_ORACLE_SYM { $$= &sp_handler_package_spec; } | PACKAGE_ORACLE_SYM BODY_ORACLE_SYM { $$= &sp_handler_package_body; } + | PACKAGE_MARIADB_SYM { $$= &sp_handler_package_spec; } + | PACKAGE_MARIADB_SYM BODY_MARIADB_SYM { $$= &sp_handler_package_body; }
Why not to use here PACKAGE_ALLMODES_SYM and BODY_ALLMODES_SYM? (also, I'd personally call them simply PACKAGE_SYM and BODY_SYM)
I checked - this will cause one more shift/reduce :(
;
@@ -19428,35 +19489,33 @@ create_routine: (Item_result) $8, $10)) MYSQL_YYABORT; } - | create_or_replace definer_opt PACKAGE_ORACLE_SYM + | create_or_replace definer_opt PACKAGE_ALLMODES_SYM opt_if_not_exists sp_name opt_create_package_chistics_init { sp_package *pkg; if (unlikely(!(pkg= Lex-> - create_package_start(thd, - SQLCOM_CREATE_PACKAGE, - &sp_handler_package_spec, - $5, $1 | $4)))) + create_package_start(thd, &sp_handler_package_spec, + $5, $1 | $4, + Lex->sp_chistics)))) MYSQL_YYABORT; pkg->set_c_chistics(Lex->sp_chistics);
Looks redundant
Thanks. Good catch. Fixed.
Lex->sphead->set_body_start(thd, YYLIP->get_cpp_tok_start()); } sp_tail_is opt_package_specification_element_list END - remember_end_opt opt_sp_name + remember_end_opt opt_trailing_sp_name { if (unlikely(Lex->create_package_finalize(thd, $5, $12, $11))) MYSQL_YYABORT; } - | create_or_replace definer_opt PACKAGE_ORACLE_SYM BODY_ORACLE_SYM + | create_or_replace definer_opt PACKAGE_ALLMODES_SYM BODY_ALLMODES_SYM opt_if_not_exists sp_name opt_create_package_chistics_init { sp_package *pkg; if (unlikely(!(pkg= Lex-> - create_package_start(thd, - SQLCOM_CREATE_PACKAGE_BODY, - &sp_handler_package_body, - $6, $1 | $5)))) + create_package_start(thd, &sp_handler_package_body, + $6, $1 | $5, + Lex->sp_chistics)))) MYSQL_YYABORT; pkg->set_c_chistics(Lex->sp_chistics);
same
Fixed.
Lex->sphead->set_body_start(thd, YYLIP->get_cpp_tok_start());
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi, Alexander, Thanks, this is ok to push. But I still want to understand (question below) On Nov 10, Alexander Barkov wrote:
On 11/7/23 10:39 PM, Sergei Golubchik wrote:
Hi, Alexander,
Looks great! Just a couple of questions below
On Nov 07, Alexander Barkov wrote:
revision-id: 28deba0a611 (mariadb-11.0.1-239-g28deba0a611) parent(s): e9573c05965 author: Alexander Barkov committer: Alexander Barkov timestamp: 2023-09-22 14:38:52 +0400 message:
MDEV-32101 CREATE PACKAGE [BODY] for sql_mode=DEFAULT
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index af2ec678daa..fca39b63312 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1285,7 +1285,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); %left TRANSACTION_SYM TIMESTAMP PERIOD_SYM SYSTEM USER COMMENT_SYM
%left PREC_BELOW_SP_OBJECT_TYPE -%left FUNCTION_SYM +%left PACKAGE_MARIADB_SYM FUNCTION_SYM
Why PACKAGE_ORACLE_SYM and PACKAGE_ALLMODES_SYM don't need it? (same for BODY_ORACLE_SYM and BODY_ALLMODES_SYM)
Keywords PACKAGE and BODY have different "reserved-ness" for sql_mode=DEFAULT and sql_mode=ORACLE.
@@ -3049,6 +3059,8 @@ sp_handler: | PROCEDURE_SYM { $$= &sp_handler_procedure; } | PACKAGE_ORACLE_SYM { $$= &sp_handler_package_spec; } | PACKAGE_ORACLE_SYM BODY_ORACLE_SYM { $$= &sp_handler_package_body; } + | PACKAGE_MARIADB_SYM { $$= &sp_handler_package_spec; } + | PACKAGE_MARIADB_SYM BODY_MARIADB_SYM { $$= &sp_handler_package_body; }
Why not to use here PACKAGE_ALLMODES_SYM and BODY_ALLMODES_SYM? (also, I'd personally call them simply PACKAGE_SYM and BODY_SYM)
I checked - this will cause one more shift/reduce :(
why, because of this different "reserved-ness" ? or because of precedence tuning? Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik