WL#1218 "Triggers"

After review and after merge fixes.


mysql-test/t/trigger.test:
  After merge fix. Updated error codes.
sql/sp_head.cc:
  After merge fix.
  To give some chances for functions/triggers we have to close tables during sp_instr_* 
  execution only if we have opened them before.
sql/sp_head.h:
  After merge fix. sp_instr constructor now takes one more argument.
sql/sql_trigger.cc:
  After merge and review fixes.
  Some variable renaming and optimizations.
sql/sql_yacc.yy:
  After merge fixes.
  sp_instr_* classes now require sp context as constructor parameter.
  Also we should be careful with adding table for which we are creating trigger to table 
  list. Some elements in trigger body can damage LEX::query_tables and so we should add this
  table to list only after parsing trigger body.

4 files changed, 55 insertions, 43 deletions

diff --git a/sql/sp_head.cc b/sql/sp_head.cc
index 3ff5f06103f..176a612d33d 100644
--- a/sql/sp_head.cc
+++ b/sql/sp_head.cc
@@ -1230,7 +1230,7 @@ sp_instr_set::execute(THD *thd, uint *nextp)
     thd->spcont->set_item(m_offset, it);
   }
   *nextp = m_ip+1;
-  if (thd->lock || thd->open_tables || thd->derived_tables)
+  if (tables && (thd->lock || thd->open_tables || thd->derived_tables))
     close_thread_tables(thd);
   DBUG_RETURN(res);
 }
@@ -1387,7 +1387,7 @@ sp_instr_jump_if::execute(THD *thd, uint *nextp)
     else
       *nextp = m_ip+1;
   }
-  if (thd->lock || thd->open_tables || thd->derived_tables)
+  if (tables && (thd->lock || thd->open_tables || thd->derived_tables))
     close_thread_tables(thd);
   DBUG_RETURN(res);
 }
@@ -1444,7 +1444,7 @@ sp_instr_jump_if_not::execute(THD *thd, uint *nextp)
     else
       *nextp = m_ip+1;
   }
-  if (thd->lock || thd->open_tables || thd->derived_tables)
+  if (tables && (thd->lock || thd->open_tables || thd->derived_tables))
     close_thread_tables(thd);
   DBUG_RETURN(res);
 }
diff --git a/sql/sp_head.h b/sql/sp_head.h
index a253d9edcbe..f4ed3760b94 100644
--- a/sql/sp_head.h
+++ b/sql/sp_head.h
@@ -392,8 +392,8 @@ class sp_instr_set_user_var : public sp_instr
 
 public:
 
-  sp_instr_set_user_var(uint ip, LEX_STRING var, Item *val)
-    : sp_instr(ip), m_set_var_item(var, val)
+  sp_instr_set_user_var(uint ip, sp_pcontext *ctx, LEX_STRING var, Item *val)
+    : sp_instr(ip, ctx), m_set_var_item(var, val)
   {}
 
   virtual ~sp_instr_set_user_var()
@@ -419,8 +419,9 @@ class sp_instr_set_trigger_field : public sp_instr
 
 public:
 
-  sp_instr_set_trigger_field(uint ip, LEX_STRING field_name, Item *val)
-    : sp_instr(ip),
+  sp_instr_set_trigger_field(uint ip, sp_pcontext *ctx,
+                             LEX_STRING field_name, Item *val)
+    : sp_instr(ip, ctx),
       trigger_field(Item_trigger_field::NEW_ROW, field_name.str),
       value(val)
   {}
diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
index 5e08192d0c1..56294b9bc80 100644
--- a/sql/sql_trigger.cc
+++ b/sql/sql_trigger.cc
@@ -319,18 +319,18 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
     if (!strncmp(triggers_file_type.str, parser->type()->str,
                  parser->type()->length))
     {
-      int i;
-      Table_triggers_list *triggers_info=
+      Field **fld, **old_fld;
+      Table_triggers_list *triggers=
         new (&table->mem_root) Table_triggers_list();
 
-      if (!triggers_info)
+      if (!triggers)
         DBUG_RETURN(1);
 
-      if (parser->parse((gptr)triggers_info, &table->mem_root,
+      if (parser->parse((gptr)triggers, &table->mem_root,
                         triggers_file_parameters, 1))
         DBUG_RETURN(1);
 
-      table->triggers= triggers_info;
+      table->triggers= triggers;
 
       /*
         We have to prepare array of Field objects which will represent OLD.*
@@ -338,27 +338,26 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
 
         TODO: This could be avoided if there is no ON UPDATE trigger.
       */
-      if (!(triggers_info->old_field=
+      if (!(triggers->old_field=
               (Field **)alloc_root(&table->mem_root, (table->fields + 1) *
                                                      sizeof(Field*))))
         DBUG_RETURN(1);
 
-      for (i= 0; i < table->fields; i++)
+      for (fld= table->field, old_fld= triggers->old_field; *fld;
+           fld++, old_fld++)
       {
         /*
           QQ: it is supposed that it is ok to use this function for field
               cloning...
         */
-        if (!(triggers_info->old_field[i]=
-                table->field[i]->new_field(&table->mem_root, table)))
+        if (!(*old_fld= (*fld)->new_field(&table->mem_root, table)))
           DBUG_RETURN(1);
-        triggers_info->old_field[i]->move_field((my_ptrdiff_t)
-                                                (table->record[1] -
-                                                 table->record[0]));
+        (*old_fld)->move_field((my_ptrdiff_t)(table->record[1] -
+                                              table->record[0]));
       }
-      triggers_info->old_field[i]= 0;
+      *old_fld= 0;
 
-      List_iterator_fast<LEX_STRING> it(triggers_info->definitions_list);
+      List_iterator_fast<LEX_STRING> it(triggers->definitions_list);
       LEX_STRING *trg_create_str, *trg_name_str;
       char *trg_name_buff;
       LEX *old_lex= thd->lex, lex;
@@ -367,8 +366,8 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
 
       while ((trg_create_str= it++))
       {
-        lex_start(thd, (uchar*)trg_create_str->str, trg_create_str->length);
-        mysql_init_query(thd, true);
+        mysql_init_query(thd, (uchar*)trg_create_str->str,
+                         trg_create_str->length, true);
         lex.trg_table= table;
         if (yyparse((void *)thd) || thd->is_fatal_error)
         {
@@ -385,7 +384,7 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
           goto err_with_lex_cleanup;
         }
 
-        triggers_info->bodies[lex.trg_chistics.event]
+        triggers->bodies[lex.trg_chistics.event]
                              [lex.trg_chistics.action_time]= lex.sphead;
         lex.sphead= 0;
 
@@ -404,7 +403,7 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
 	old_global_mem_root= my_pthread_getspecific_ptr(MEM_ROOT*, THR_MALLOC);
 	my_pthread_setspecific_ptr(THR_MALLOC, &table->mem_root);
 
-        if (triggers_info->names_list.push_back(trg_name_str))
+        if (triggers->names_list.push_back(trg_name_str))
           goto err_with_lex_cleanup;
 
         my_pthread_setspecific_ptr(THR_MALLOC, old_global_mem_root);
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index 41432cee673..0f65aaa70da 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -1212,15 +1212,6 @@ create:
             LEX *lex= Lex;
             sp_head *sp;
            
-            lex->name_and_length= $3;
-           
-            /* QQ: Could we loosen lock type in certain cases ? */
-            if (!lex->select_lex.add_table_to_list(YYTHD, $7, 
-                                                   (LEX_STRING*) 0,
-                                                   TL_OPTION_UPDATING,
-                                                   TL_WRITE))
-              YYABORT;
-              
             if (lex->sphead)
             {
               net_printf(YYTHD, ER_SP_NO_RECURSIVE_CREATE, "TRIGGER");
@@ -1256,6 +1247,23 @@ create:
             if (sp->m_old_cmq)
               YYTHD->client_capabilities |= CLIENT_MULTI_QUERIES;
             sp->restore_thd_mem_root(YYTHD);
+
+            lex->name_and_length= $3;
+
+            /*
+              We have to do it after parsing trigger body, because some of
+              sp_proc_stmt alternatives are not saving/restoring LEX, so
+              lex->query_tables can be wiped out.
+              
+              QQ: What are other consequences of this?
+              
+              QQ: Could we loosen lock type in certain cases ?
+            */
+            if (!lex->select_lex.add_table_to_list(YYTHD, $7, 
+                                                   (LEX_STRING*) 0,
+                                                   TL_OPTION_UPDATING,
+                                                   TL_WRITE))
+              YYABORT;
           }
 	;
 
@@ -6922,10 +6930,18 @@ option_value:
                 We have to use special instruction in functions and triggers
                 because sp_instr_stmt will close all tables and thus ruin
                 execution of statement invoking function or trigger.
+
+                We also do not want to allow expression with subselects in
+                this case.
               */
+              if (lex->query_tables)
+              {
+                send_error(YYTHD, ER_SP_SUBSELECT_NYI);
+                YYABORT;
+              }
               sp_instr_set_user_var *i= 
-                new sp_instr_set_user_var(lex->sphead->instructions(), 
-                                          $2, $4);
+                new sp_instr_set_user_var(lex->sphead->instructions(),
+                                          lex->spcont, $2, $4);
 	      lex->sphead->add_instr(i);
             }
             else
@@ -6941,12 +6957,8 @@ option_value:
               /* We are in trigger and assigning value to field of new row */
               Item *it;
               sp_instr_set_trigger_field *i;
-              if ($3 && $3->type() == Item::SUBSELECT_ITEM)
-              {  /*
-                   QQ For now, just disallow subselects as values
-                   Unfortunately this doesn't helps in case when we have
-                   subselect deeper in expression.
-                 */
+              if (lex->query_tables)
+              {
                 send_error(YYTHD, ER_SP_SUBSELECT_NYI);
                 YYABORT;
               }
@@ -6958,7 +6970,7 @@ option_value:
                 it= new Item_null();
               }
               i= new sp_instr_set_trigger_field(lex->sphead->instructions(),
-                                                $1.base_name, it);
+                                                lex->spcont, $1.base_name, it);
               if (lex->trg_table && i->setup_field(YYTHD, lex->trg_table,
                                                    lex->trg_chistics.event))
               {