Addressing Monty's review suggestions for MDEV-11952 Oracle-style packages (partial)

- Using array_elements() instead of a constant to iterate through an array
- Adding some comments
- Adding new-line function comments
- Using STRING_WITH_LEN instead of C_STRING_WITH_LEN

8 files changed, 99 insertions, 30 deletions

diff --git a/client/mysqldump.c b/client/mysqldump.c
index 694ce4a5134..dc87338aac2 100644
--- a/client/mysqldump.c
+++ b/client/mysqldump.c
@@ -2514,7 +2514,7 @@ static uint dump_routines_for_db(char *db)
                                      "Create Package Body"};
   char       db_name_buff[NAME_LEN*2+3], name_buff[NAME_LEN*2+3];
   char       *routine_name;
-  int        i;
+  uint       i;
   FILE       *sql_file= md_result_file;
   MYSQL_ROW  row, routine_list_row;
 
@@ -2550,7 +2550,7 @@ static uint dump_routines_for_db(char *db)
     fputs("\t<routines>\n", sql_file);
 
   /* 0, retrieve and dump functions, 1, procedures, etc. */
-  for (i= 0; i < 4; i++)
+  for (i= 0; i < array_elements(routine_type); i++)
   {
     my_snprintf(query_buff, sizeof(query_buff),
                 "SHOW %s STATUS WHERE Db = '%s'",
diff --git a/sql/item.h b/sql/item.h
index df6c922f300..dcbe41de97f 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -368,11 +368,37 @@ typedef enum monotonicity_info
 
 class sp_rcontext;
 
+/**
+  A helper class to collect different behavior of various kinds of SP variables:
+  - local SP variables and SP parameters
+  - PACKAGE BODY routine variables
+  - (there will be more kinds in the future)
+*/
+
 class Sp_rcontext_handler
 {
 public:
   virtual ~Sp_rcontext_handler() {}
+  /**
+    A prefix used for SP variable names in queries:
+    - EXPLAIN EXTENDED
+    - SHOW PROCEDURE CODE
+    Local variables and SP parameters have empty prefixes.
+    Package body variables are marked with a special prefix.
+    This improves readability of the output of these queries,
+    especially when a local variable or a parameter has the same
+    name with a package body variable.
+  */
   virtual const LEX_CSTRING *get_name_prefix() const= 0;
+  /**
+    At execution time THD->spcont points to the run-time context (sp_rcontext)
+    of the currently executed routine.
+    Local variables store their data in the sp_rcontext pointed by thd->spcont.
+    Package body variables store data in separate sp_rcontext that belongs
+    to the package.
+    This method provides access to the proper sp_rcontext structure,
+    depending on the SP variable kind.
+  */
   virtual sp_rcontext *get_rcontext(sp_rcontext *ctx) const= 0;
 };
 
diff --git a/sql/item_xmlfunc.cc b/sql/item_xmlfunc.cc
index 0b9e515a61f..63cc07fa34c 100644
--- a/sql/item_xmlfunc.cc
+++ b/sql/item_xmlfunc.cc
@@ -2632,6 +2632,10 @@ my_xpath_parse_VariableReference(MY_XPATH *xpath)
     sp_variable *spv;
     const Sp_rcontext_handler *rh;
     LEX *lex;
+    /*
+      We call lex->find_variable() rather than thd->lex->spcont->find_variable()
+      to make sure package body variables are properly supported.
+    */
     if ((lex= thd->lex) &&
         (spv= lex->find_variable(&name, &rh)))
     {
diff --git a/sql/sp.cc b/sql/sp.cc
index 1d0f32a2888..af86737ebb9 100644
--- a/sql/sp.cc
+++ b/sql/sp.cc
@@ -780,6 +780,7 @@ Sp_handler::db_find_and_cache_routine(THD *thd,
   Silence DEPRECATED SYNTAX warnings when loading a stored procedure
   into the cache.
 */
+
 struct Silence_deprecated_warning : public Internal_error_handler
 {
 public:
@@ -1002,7 +1003,7 @@ Sp_handler::db_load_routine(THD *thd, const Database_qualified_name *name,
 
     if (type() == TYPE_ENUM_PACKAGE_BODY)
     {
-      sp_package *package= sphp[0]->get_package();
+      sp_package *package= (*sphp)->get_package();
       List_iterator<LEX> it(package->m_routine_implementations);
       for (LEX *lex; (lex= it++); )
       {
@@ -1078,6 +1079,7 @@ sp_returns_type(THD *thd, String &result, const sp_head *sp)
   @return       SP_OK on success, or SP_DELETE_ROW_FAILED on error.
   used to indicate about errors.
 */
+
 int
 Sp_handler::sp_drop_routine_internal(THD *thd,
                                      const Database_qualified_name *name,
@@ -1111,7 +1113,7 @@ Sp_handler::sp_find_and_drop_routine(THD *thd, TABLE *table,
                                      const Database_qualified_name *name) const
 {
   int ret;
-  if (SP_OK != (ret= db_find_routine_aux(thd, name, table)))
+  if ((ret= db_find_routine_aux(thd, name, table)) != SP_OK)
     return ret;
   return sp_drop_routine_internal(thd, name, table);
 }
@@ -1123,11 +1125,23 @@ Sp_handler_package_spec::
                            const Database_qualified_name *name) const
 {
   int ret;
-  if (SP_OK != (ret= db_find_routine_aux(thd, name, table)))
+  if ((ret= db_find_routine_aux(thd, name, table)) != SP_OK)
     return ret;
+  /*
+    When we do "DROP PACKAGE pkg", we should also perform
+    "DROP PACKAGE BODY pkg" automatically.
+  */
   ret= sp_handler_package_body.sp_find_and_drop_routine(thd, table, name);
   if (ret != SP_KEY_NOT_FOUND && ret != SP_OK)
-    return ret;
+  {
+    /*
+      - SP_KEY_NOT_FOUND means that "CREATE PACKAGE pkg" did not
+        have a correspoinding "CREATE PACKAGE BODY pkg" yet.
+      - SP_OK means that "CREATE PACKAGE pkg" had a correspoinding
+        "CREATE PACKAGE BODY pkg", which was successfully dropped.
+    */
+    return ret; // Other codes mean an unexpecte error
+  }
   return Sp_handler::sp_find_and_drop_routine(thd, table, name);
 }
 
@@ -1231,7 +1245,7 @@ Sp_handler::sp_create_routine(THD *thd, const sp_head *sp) const
           DBUG_ASSERT(0);
           ret= SP_OK;
         }
-        if (ret)
+        if (ret != SP_OK)
           goto done;
       }
       else if (lex->create_info.if_not_exists())
@@ -1565,7 +1579,7 @@ Sp_handler::sp_drop_routine(THD *thd,
   if (!(table= open_proc_table_for_update(thd)))
     DBUG_RETURN(SP_OPEN_TABLE_FAILED);
 
-  if (SP_OK == (ret= sp_find_and_drop_routine(thd, table, name)) &&
+  if ((ret= sp_find_and_drop_routine(thd, table, name)) == SP_OK &&
       write_bin_log(thd, TRUE, thd->query(), thd->query_length()))
     ret= SP_INTERNAL_ERROR;
   /*
@@ -1916,6 +1930,7 @@ Sp_handler::sp_show_create_routine(THD *thd,
   and return it as a 0-terminated string
     'pkg.name' -> 'pkg\0'
 */
+
 class Prefix_name_buf: public LEX_CSTRING
 {
   char m_buf[SAFE_NAME_LEN + 1];
@@ -1948,6 +1963,7 @@ public:
   - either returns the original SP,
   - or makes and returns a new clone of SP
 */
+
 sp_head *
 Sp_handler::sp_clone_and_link_routine(THD *thd,
                                       const Database_qualified_name *name,
@@ -2015,7 +2031,7 @@ Sp_handler::sp_clone_and_link_routine(THD *thd,
       1. Cut the package name prefix from the routine name: 'pkg1.p1' -> 'p1',
          to have db_load_routine() generate and parse a query like this:
            CREATE PROCEDURE p1 ...;
-         rether than:
+         rather than:
            CREATE PROCEDURE pkg1.p1 ...;
          The latter would be misinterpreted by the parser as a standalone
          routine 'p1' in the database 'pkg1', which is not what we need.
@@ -2126,6 +2142,7 @@ Sp_handler::sp_find_routine(THD *thd, const Database_qualified_name *name,
   @retval non-NULL   - a pointer to an sp_head object
   @retval NULL       - an error happened.
 */
+
 sp_head *
 Sp_handler::sp_find_package_routine(THD *thd,
                                     const LEX_CSTRING pkgname_str,
@@ -2168,6 +2185,7 @@ Sp_handler::sp_find_package_routine(THD *thd,
   @retval non-NULL  - a pointer to an sp_head object
   @retval NULL      - an error happened
 */
+
 sp_head *
 Sp_handler::sp_find_package_routine(THD *thd,
                                     const Database_qualified_name *name,
@@ -2301,6 +2319,7 @@ bool sp_add_used_routine(Query_tables_list *prelocking_ctx, Query_arena *arena,
   It's used during parsing of CREATE PACKAGE BODY,
   to load the corresponding CREATE PACKAGE.
 */
+
 int
 Sp_handler::sp_cache_routine_reentrant(THD *thd,
                                        const Database_qualified_name *name,
@@ -2315,7 +2334,7 @@ Sp_handler::sp_cache_routine_reentrant(THD *thd,
 }
 
 
-/*
+/**
   Check if a routine has a declaration in the CREATE PACKAGE statement,
   by looking up in thd->sp_package_spec_cache, and by loading from mysql.proc
   if needed.
@@ -2340,6 +2359,7 @@ Sp_handler::sp_cache_routine_reentrant(THD *thd,
   After the call of this function, the package specification is always cached,
   unless a fatal error happens.
 */
+
 static bool
 is_package_public_routine(THD *thd,
                           const LEX_CSTRING &db,
@@ -2356,7 +2376,7 @@ is_package_public_routine(THD *thd,
 }
 
 
-/*
+/**
   Check if a routine has a declaration in the CREATE PACKAGE statement
   by looking up in sp_package_spec_cache.
 
@@ -2373,6 +2393,7 @@ is_package_public_routine(THD *thd,
   The package specification (i.e. the CREATE PACKAGE statement) for
   the current package body must already be loaded and cached at this point.
 */
+
 static bool
 is_package_public_routine_quick(THD *thd,
                                 const LEX_CSTRING &db,
@@ -2388,10 +2409,11 @@ is_package_public_routine_quick(THD *thd,
 }
 
 
-/*
+/**
   Check if a qualified name, e.g. "CALL name1.name2",
   refers to a known routine in the package body "pkg".
 */
+
 static bool
 is_package_body_routine(THD *thd, sp_package *pkg,
                         const LEX_CSTRING &name1,
@@ -2404,11 +2426,12 @@ is_package_body_routine(THD *thd, sp_package *pkg,
 }
 
 
-/*
+/**
   Resolve a qualified routine reference xxx.yyy(), between:
   - A standalone routine: xxx.yyy
   - A package routine:    current_database.xxx.yyy
 */
+
 bool Sp_handler::
   sp_resolve_package_routine_explicit(THD *thd,
                                       sp_head *caller,
@@ -2444,11 +2467,12 @@ bool Sp_handler::
 }
 
 
-/*
+/**
   Resolve a non-qualified routine reference yyy(), between:
   - A standalone routine: current_database.yyy
   - A package routine:    current_database.current_package.yyy
 */
+
 bool Sp_handler::
   sp_resolve_package_routine_implicit(THD *thd,
                                       sp_head *caller,
@@ -2534,6 +2558,7 @@ bool Sp_handler::
   @retval         false   on success
   @retval         true    on error (e.g. EOM, could not read CREATE PACKAGE)
 */
+
 bool
 Sp_handler::sp_resolve_package_routine(THD *thd,
                                        sp_head *caller,
@@ -2813,6 +2838,7 @@ int Sp_handler::sp_cache_routine(THD *thd,
   @retval false      - loaded or does not exists
   @retval true       - error while loading mysql.proc
 */
+
 int
 Sp_handler::sp_cache_package_routine(THD *thd,
                                      const LEX_CSTRING &pkgname_cstr,
@@ -2856,6 +2882,7 @@ Sp_handler::sp_cache_package_routine(THD *thd,
   @retval false      - loaded or does not exists
   @retval true       - error while loading mysql.proc
 */
+
 int Sp_handler::sp_cache_package_routine(THD *thd,
                                          const Database_qualified_name *name,
                                          bool lookup_only, sp_head **sp) const
@@ -2873,6 +2900,7 @@ int Sp_handler::sp_cache_package_routine(THD *thd,
   @return
     Returns false on success, true on (alloc) failure.
 */
+
 bool
 Sp_handler::show_create_sp(THD *thd, String *buf,
                            const LEX_CSTRING &db,
@@ -3015,15 +3043,15 @@ Sp_handler::sp_load_for_information_schema(THD *thd, TABLE *proc_table,
 
 LEX_CSTRING Sp_handler_procedure::empty_body_lex_cstring(sql_mode_t mode) const
 {
-  static LEX_CSTRING m_empty_body_std= {C_STRING_WITH_LEN("BEGIN END")};
-  static LEX_CSTRING m_empty_body_ora= {C_STRING_WITH_LEN("AS BEGIN NULL; END")};
+  static LEX_CSTRING m_empty_body_std= {STRING_WITH_LEN("BEGIN END")};
+  static LEX_CSTRING m_empty_body_ora= {STRING_WITH_LEN("AS BEGIN NULL; END")};
   return mode & MODE_ORACLE ? m_empty_body_ora : m_empty_body_std;
 }
 
 
 LEX_CSTRING Sp_handler_function::empty_body_lex_cstring(sql_mode_t mode) const
 {
-  static LEX_CSTRING m_empty_body_std= {C_STRING_WITH_LEN("RETURN NULL")};
-  static LEX_CSTRING m_empty_body_ora= {C_STRING_WITH_LEN("AS BEGIN RETURN NULL; END")};
+  static LEX_CSTRING m_empty_body_std= {STRING_WITH_LEN("RETURN NULL")};
+  static LEX_CSTRING m_empty_body_ora= {STRING_WITH_LEN("AS BEGIN RETURN NULL; END")};
   return mode & MODE_ORACLE ? m_empty_body_ora : m_empty_body_std;
 }
diff --git a/sql/sp.h b/sql/sp.h
index 3fb20b1c8e8..380dd69d3a1 100644
--- a/sql/sp.h
+++ b/sql/sp.h
@@ -371,12 +371,12 @@ public:
   stored_procedure_type type() const { return TYPE_ENUM_PACKAGE; }
   LEX_CSTRING type_lex_cstring() const
   {
-    static LEX_CSTRING m_type_str= {C_STRING_WITH_LEN("PACKAGE")};
+    static LEX_CSTRING m_type_str= {STRING_WITH_LEN("PACKAGE")};
     return m_type_str;
   }
   LEX_CSTRING empty_body_lex_cstring(sql_mode_t mode) const
   {
-    static LEX_CSTRING m_empty_body= {C_STRING_WITH_LEN("BEGIN END")};
+    static LEX_CSTRING m_empty_body= {STRING_WITH_LEN("BEGIN END")};
     return m_empty_body;
   }
   const char *show_create_routine_col1_caption() const
@@ -404,12 +404,12 @@ public:
   stored_procedure_type type() const { return TYPE_ENUM_PACKAGE_BODY; }
   LEX_CSTRING type_lex_cstring() const
   {
-    static LEX_CSTRING m_type_str= {C_STRING_WITH_LEN("PACKAGE BODY")};
+    static LEX_CSTRING m_type_str= {STRING_WITH_LEN("PACKAGE BODY")};
     return m_type_str;
   }
   LEX_CSTRING empty_body_lex_cstring(sql_mode_t mode) const
   {
-    static LEX_CSTRING m_empty_body= {C_STRING_WITH_LEN("BEGIN END")};
+    static LEX_CSTRING m_empty_body= {STRING_WITH_LEN("BEGIN END")};
     return m_empty_body;
   }
   const char *show_create_routine_col1_caption() const
diff --git a/sql/sp_head.cc b/sql/sp_head.cc
index ac88f05a866..151023aa24c 100644
--- a/sql/sp_head.cc
+++ b/sql/sp_head.cc
@@ -369,6 +369,7 @@ Item *THD::sp_prepare_func_item(Item **it_addr, uint cols)
 /**
   Fix an Item for evaluation for SP.
 */
+
 Item *THD::sp_fix_func_item(Item **it_addr)
 {
   DBUG_ENTER("THD::sp_fix_func_item");
@@ -594,6 +595,7 @@ sp_package::~sp_package()
 /*
   Test if two routines have equal specifications
 */
+
 bool sp_head::eq_routine_spec(const sp_head *sp) const
 {
   // TODO: Add tests for equal return data types (in case of FUNCTION)
@@ -1607,6 +1609,7 @@ bool sp_head::check_execute_access(THD *thd) const
   @retval           NULL - error (access denided or EOM)
   @retval          !NULL - success (the invoker has rights to all %TYPE tables)
 */
+
 sp_rcontext *sp_head::rcontext_create(THD *thd, Field *ret_value,
                                       Row_definition_list *defs,
                                       bool switch_security_ctx)
@@ -1784,6 +1787,7 @@ err_with_cleanup:
 /*
   Execute the package initialization section.
 */
+
 bool sp_package::instantiate_if_needed(THD *thd)
 {
   List<Item> args;
@@ -2459,6 +2463,7 @@ sp_head::merge_lex(THD *thd, LEX *oldlex, LEX *sublex)
 /**
   Put the instruction on the backpatch list, associated with the label.
 */
+
 int
 sp_head::push_backpatch(THD *thd, sp_instr *i, sp_label *lab,
                         List<bp_t> *list, backpatch_instr_type itype)
@@ -2514,6 +2519,7 @@ sp_head::push_backpatch_goto(THD *thd, sp_pcontext *ctx, sp_label *lab)
   Update all instruction with this label in the backpatch list to
   the current position.
 */
+
 void
 sp_head::backpatch(sp_label *lab)
 {
@@ -3041,6 +3047,7 @@ bool sp_head::add_instr_preturn(THD *thd, sp_pcontext *spcont)
 
   QQ: Perhaps we need a dedicated sp_instr_nop for this purpose.
 */
+
 bool sp_head::replace_instr_to_nop(THD *thd, uint ip)
 {
   sp_instr *instr= get_instr(ip);
@@ -3165,6 +3172,7 @@ sp_head::opt_mark()
   @return
     0 if ok, !=0 on error.
 */
+
 int
 sp_head::show_routine_code(THD *thd)
 {
@@ -4457,6 +4465,7 @@ sp_instr_agg_cfetch::print(String *str)
   - opens the cursor without copying data (materialization).
   - copies the cursor structure to the associated %ROWTYPE variable.
 */
+
 int
 sp_instr_cursor_copy_struct::exec_core(THD *thd, uint *nextp)
 {
@@ -4933,6 +4942,7 @@ sp_head::set_local_variable(THD *thd, sp_pcontext *spcont,
 /**
   Similar to set_local_variable(), but for ROW variable fields.
 */
+
 bool
 sp_head::set_local_variable_row_field(THD *thd, sp_pcontext *spcont,
                                       const Sp_rcontext_handler *rh,
@@ -5127,6 +5137,7 @@ bool sp_head::spvar_fill_table_rowtype_reference(THD *thd,
     END p1;
   Check that the first p1 and the last p1 match.
 */
+
 bool sp_head::check_package_routine_end_name(const LEX_CSTRING &end_name) const
 {
   LEX_CSTRING non_qualified_name= m_name;
diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc
index 7c8c676d337..6166d1d9615 100644
--- a/sql/sp_rcontext.cc
+++ b/sql/sp_rcontext.cc
@@ -52,7 +52,7 @@ const LEX_CSTRING *Sp_rcontext_handler_local::get_name_prefix() const
 const LEX_CSTRING *Sp_rcontext_handler_package_body::get_name_prefix() const
 {
   static const LEX_CSTRING sp_package_body_variable_prefix_clex_str=
-                           {C_STRING_WITH_LEN("PACKAGE_BODY.")};
+                           {STRING_WITH_LEN("PACKAGE_BODY.")};
   return &sp_package_body_variable_prefix_clex_str;
 }
 
diff --git a/sql/sql_lex.h b/sql/sql_lex.h
index 2517fb6b544..bfb201185b1 100644
--- a/sql/sql_lex.h
+++ b/sql/sql_lex.h
@@ -319,13 +319,13 @@ enum enum_sp_aggregate_type
   GROUP_AGGREGATE
 };
 
-const LEX_STRING sp_data_access_name[]=
+const LEX_CSTRING sp_data_access_name[]=
 {
-  { C_STRING_WITH_LEN("") },
-  { C_STRING_WITH_LEN("CONTAINS SQL") },
-  { C_STRING_WITH_LEN("NO SQL") },
-  { C_STRING_WITH_LEN("READS SQL DATA") },
-  { C_STRING_WITH_LEN("MODIFIES SQL DATA") }
+  { STRING_WITH_LEN("") },
+  { STRING_WITH_LEN("CONTAINS SQL") },
+  { STRING_WITH_LEN("NO SQL") },
+  { STRING_WITH_LEN("READS SQL DATA") },
+  { STRING_WITH_LEN("MODIFIES SQL DATA") }
 };
 
 #define DERIVED_SUBQUERY        1
@@ -3685,7 +3685,7 @@ public:
                                         Item *value);
   sp_variable *sp_add_for_loop_upper_bound(THD *thd, Item *value)
   {
-    LEX_CSTRING name= { C_STRING_WITH_LEN("[upper_bound]") };
+    LEX_CSTRING name= { STRING_WITH_LEN("[upper_bound]") };
     return sp_add_for_loop_variable(thd, &name, value);
   }
   bool sp_for_loop_intrange_declarations(THD *thd, Lex_for_loop_st *loop,