A cleanup for MDEV-10914 ROW data type for stored routine variables

Addressing Monty's review suggestions

mysql-test/suite/compat/oracle/r/binlog_stm_sp.result

@@ -187,6 +187,7 @@ rec t1%ROWTYPE;
 BEGIN
 rec.a:=100;
 rec.b:=200;
+SELECT rec=ROW(100,200) AS true1, ROW(100,200)=rec AS true2;
 INSERT INTO t1 VALUES (rec.a,rec.b);
 INSERT INTO t1 VALUES (10, rec=ROW(100,200));
 INSERT INTO t1 VALUES (10, ROW(100,200)=rec);
@@ -207,6 +208,8 @@ INSERT INTO t1 SELECT 12, 21 FROM DUAL WHERE ROW(100,200)=rec;
 END;
 $$
 CALL p1();
+true1	true2
+1	1
 SELECT * FROM t1;
 a	b
 100	200
@@ -261,6 +264,7 @@ rec t1%ROWTYPE;
 BEGIN
 rec.a:=100;
 rec.b:=200;
+SELECT rec=ROW(100,200) AS true1, ROW(100,200)=rec AS true2;
 INSERT INTO t1 VALUES (rec.a,rec.b);
 INSERT INTO t1 VALUES (10, rec=ROW(100,200));
 INSERT INTO t1 VALUES (10, ROW(100,200)=rec);

mysql-test/suite/compat/oracle/t/binlog_stm_sp.test

@@ -93,6 +93,7 @@ AS
 BEGIN
   rec.a:=100;
   rec.b:=200;
+  SELECT rec=ROW(100,200) AS true1, ROW(100,200)=rec AS true2;
   INSERT INTO t1 VALUES (rec.a,rec.b);
   INSERT INTO t1 VALUES (10, rec=ROW(100,200));
   INSERT INTO t1 VALUES (10, ROW(100,200)=rec);

sql/item.cc

@@ -1789,30 +1789,32 @@ bool Item_splocal_row_field::set_value(THD *thd, sp_rcontext *ctx, Item **it)
 bool Item_splocal_row_field_by_name::fix_fields(THD *thd, Item **it)
 {
   m_thd= thd;
-  Item *row= m_thd->spcont->get_item(m_var_idx);
+  Item *item, *row= m_thd->spcont->get_item(m_var_idx);
   if (row->element_index_by_name(&m_field_idx, m_field_name))
   {
     my_error(ER_ROW_VARIABLE_DOES_NOT_HAVE_FIELD, MYF(0),
              m_name.str, m_field_name.str);
     return true;
   }
-  Item *item= row->element_index(m_field_idx);
+  item= row->element_index(m_field_idx);
   set_handler(item->type_handler());
   return fix_fields_from_item(thd, it, item);
 }
 
 
 void Item_splocal_row_field_by_name::print(String *str, enum_query_type)
 {
-  str->reserve(m_name.length + 2 * m_field_name.length + 8);
-  str->append(m_name.str, m_name.length);
-  str->append('.');
-  str->append(m_field_name.str, m_field_name.length);
-  str->append('@');
-  str->append_ulonglong(m_var_idx);
-  str->append("[\"", 2);
-  str->append(m_field_name.str, m_field_name.length);
-  str->append("\"]", 2);
+  // +16 should be enough for .NNN@[""]
+  if (str->reserve(m_name.length + 2 * m_field_name.length + 16))
+    return;
+  str->qs_append(m_name.str, m_name.length);
+  str->qs_append('.');
+  str->qs_append(m_field_name.str, m_field_name.length);
+  str->qs_append('@');
+  str->qs_append(m_var_idx);
+  str->qs_append("[\"", 2);
+  str->qs_append(m_field_name.str, m_field_name.length);
+  str->qs_append("\"]", 2);
 }
 
 

sql/item.h

@@ -1624,7 +1624,7 @@ class Item: public Value_source,
   virtual Item* element_index(uint i) { return this; }
   virtual bool element_index_by_name(uint *idx, const LEX_STRING &name) const
   {
-    return true;
+    return true; // Error
   }
   virtual Item** addr(uint i) { return 0; }
   virtual bool check_cols(uint c);

sql/sp_head.cc

@@ -3501,11 +3501,11 @@ sp_instr_set_row_field_by_name::print(String *str)
   str->qs_append(STRING_WITH_LEN("set "));
     str->qs_append(var->name.str, var->name.length);
   str->qs_append('.');
-  str->qs_append(m_field_name.str);
+  str->qs_append(m_field_name.str, m_field_name.length);
   str->qs_append('@');
   str->qs_append(m_offset);
   str->qs_append("[\"",2);
-  str->qs_append(m_field_name.str);
+  str->qs_append(m_field_name.str, m_field_name.length);
   str->qs_append("\"]",2);
   str->qs_append(' ');
   m_value->print(str, enum_query_type(QT_ORDINARY |

sql/sp_head.h

@@ -1176,6 +1176,22 @@ class sp_instr_set_row_field : public sp_instr_set
 }; // class sp_instr_set_field : public sp_instr_set
 
 
+/**
+  This class handles assignment instructions like this:
+  DECLARE
+    CURSOR cur IS SELECT * FROM t1;
+    rec cur%ROWTYPE;
+  BEGIN
+    rec.column1:= 10; -- This instruction
+  END;
+
+  The idea is that during sp_rcontext::create() we do not know the extact
+  structure of "rec". It gets resolved at run time, during the corresponding
+  sp_instr_cursor_copy_struct::exec_core().
+
+  So sp_instr_set_row_field_by_name searches for ROW fields by name,
+  while sp_instr_set_row_field (see above) searches for ROW fields by index.
+*/
 class sp_instr_set_row_field_by_name : public sp_instr_set
 {
   // Prevent use of this

sql/sp_rcontext.cc

@@ -195,6 +195,11 @@ bool sp_rcontext::resolve_type_ref(THD *thd, Column_definition *def,
 }
 
 
+/**
+  This method resolves the structure of a variable declared as:
+     rec t1%ROWTYPE;
+  It opens the table "t1" and copies its structure to %ROWTYPE variable.
+*/
 bool sp_rcontext::resolve_table_rowtype_ref(THD *thd,
                                             Row_definition_list &defs,
                                             Table_ident *ref)
@@ -206,6 +211,11 @@ bool sp_rcontext::resolve_table_rowtype_ref(THD *thd,
   LEX *save_lex= thd->lex;
   bool rc= true;
 
+  /*
+    Create a temporary LEX on stack and switch to it.
+    In case of VIEW, open_tables_only_view_structure() will open more
+    tables/views recursively. We want to avoid them to stick to the current LEX.
+  */
   sp_lex_local lex(thd, thd->lex);
   thd->lex= &lex;
 
@@ -228,11 +238,12 @@ bool sp_rcontext::resolve_table_rowtype_ref(THD *thd,
          in the end of this method.
       */
       LEX_CSTRING tmp= {src[0]->field_name, strlen(src[0]->field_name)};
+      Spvar_definition *def;
       if ((rc= check_column_grant_for_type_ref(thd, table_list,
                                                tmp.str, tmp.length)) ||
-          (rc= !(src[0]->field_name= thd->strmake(tmp.str, tmp.length))))
+          (rc= !(src[0]->field_name= thd->strmake(tmp.str, tmp.length))) ||
+          (rc= !(def= new (thd->mem_root) Spvar_definition(thd, *src))))
         break;
-      Spvar_definition *def= new (thd->mem_root) Spvar_definition(thd, *src);
       src[0]->field_name= tmp.str; // Restore field name, just in case.
       def->flags&= (uint) ~NOT_NULL_FLAG;
       if ((rc= def->sp_prepare_create_field(thd, thd->mem_root)))
@@ -884,6 +895,16 @@ bool sp_cursor::Select_fetch_into_spvars::
 int sp_cursor::Select_fetch_into_spvars::send_data(List<Item> &items)
 {
   Item *item;
+  /*
+    If we have only one variable in spvar_list, and this is a ROW variable,
+    and the number of fields in the ROW variable matches the number of
+    fields in the query result, we fetch to this ROW variable.
+
+    If there is one variable, and it is a ROW variable, but its number
+    of fields does not match the number of fields in the query result,
+    we go through send_data_to_variable_list(). It will report an error
+    on attempt to assign a scalar value to a ROW variable.
+  */
   return spvar_list->elements == 1 &&
          (item= thd->spcont->get_item(spvar_list->head()->offset)) &&
          item->type_handler() == &type_handler_row &&

sql/sql_lex.cc

@@ -5282,6 +5282,15 @@ bool LEX::sp_variable_declarations_finalize(THD *thd, int nvars,
 }
 
 
+/**
+  Finalize a %ROWTYPE declaration, e.g.:
+    DECLARE a,b,c,d t1%ROWTYPE := ROW(1,2,3);
+
+  @param thd   - the current thd
+  @param nvars - the number of variables in the declaration
+  @param ref   - the table or cursor name (see comments below)
+  @param def   - the default value, e.g., ROW(1,2,3), or NULL (no default).
+*/
 bool
 LEX::sp_variable_declarations_rowtype_finalize(THD *thd, int nvars,
                                                Qualified_column_ident *ref,
@@ -5295,6 +5304,7 @@ LEX::sp_variable_declarations_rowtype_finalize(THD *thd, int nvars,
   if (!def && !(def= new (thd->mem_root) Item_null(thd)))
     return true;
 
+  // Loop through all variables in the same declaration
   for (uint i= num_vars - nvars; i < num_vars; i++)
   {
     bool last= i == num_vars - 1;