Hello Sergei,

Thanks for the review.

The new patch is here:

https://github.com/MariaDB/server/commit/a5344196e01239769bbf214ced0205178001c49f


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 secur...@mariadb.org

_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org

Reply via email to