Fix for bug #5888 "Triggers with nonexistent columns cause packets

out of order". (final version)

Now instead of binding Item_trigger_field to TABLE objects during
trigger definition parsing at table open, we perform pass through
special list of all such objects in trigger. This allows easily check
all references to fields in old/new version of row in trigger during
execution of CREATE TRIGGER statement (this is more courtesy for users
since we can't check everything anyway).
We also report that such reference is bad by returning error from
Item_trigger_field::fix_fields() method (instead of setup_field())
This means that if trigger is broken we will bark during trigger
execution instead of trigger definition parsing at table open.
(i.e. now we allow to open tables with broken triggers).


mysql-test/r/trigger.result:
  Added test which attempts to create trigger for table referencing to
  field which does not exist in this table.
mysql-test/t/trigger.test:
  Added test which attempts to create trigger for table referencing to
  field which does not exist in this table.
sql/item.cc:
  Item_trigger_field::setup_field() now returns void. If any error
  will occur we will report it at fix_fields() stage.
sql/item.h:
  Item_trigger_field:
  - Added next_trg_field member for linking all such objects in trigger
    in one list.
  - Also setup_field() now returns void. If any error will occur we will
    report it at fix_fields() stage.
sql/mysql_priv.h:
  Added SQL_LIST::push_back() method which allows to add another SQL_LIST
  to the end of this SQL_LIST.
sql/sp_head.cc:
  sp_head::init()/reset_lex()/restore_lex():
   In order to fill global LEX::trg_table_fields (list of all 
   Item_trigger_field objects for trigger) we should init the same list
   in LEX of substatement before its parsing and merge it to global list
   after parsing.
sql/sp_head.h:
  sp_instr_trigger_field:
    Made trigger_field member public to be able to add it more easily to
    global list of all Item_trigger_field objects in trigger.
sql/sql_lex.cc:
  LEX::trg_table was removed.
sql/sql_lex.h:
  Now we are binding Item_trigger_field's to TABLE object by passing
  through specially constructed list of all such objects in this trigger
  instead of doing this during trigger definition parsing at table open.
  So we no longer need LEX::trg_table, we use LEX::trg_table_fields list
  instead.
sql/sql_parse.cc:
  mysql_execute_command():
    Since now we use trigger body for some checks in
    mysql_create_or_drop_trigger() we should destroy it only
    after calling this function.
sql/sql_trigger.cc:
  Now instead of binding Item_trigger_field to TABLE objects during
  trigger definition parsing at table open, we perform pass through
  special list of all such objects in trigger. This allows easily check
  all references to fields in old/new version of row in trigger during
  execution of CREATE TRIGGER statement (this is more courtesy for users
  since we can't check everything anyway).
  We also report that such reference is bad by returning error from
  Item_trigger_field::fix_fields() method (instead of setup_field())
  This means that if trigger is broken we will bark during trigger
  execution instead of trigger definition parsing at table open.
  (i.e. now we allow to open tables with broken triggers).
  
  Table_triggers_list::prepare_old_row_accessors() method was added to be
  able to reuse code creating Field objects referencing TABLE::record[1]
  buffer instead of TABLE::record[0].
sql/sql_trigger.h:
  Added Table_triggers_list::prepare_old_row_accessors() method to be
  able to reuse code creating Field objects referencing to TABLE::record[1]
  instead of record[0].
sql/sql_yacc.yy:
  Now instead of performing binding of Item_trigger_field objects
  to TABLE object during trigger definition parsing at table open,
  we perform this binding by passing through specially constructed
  list of all such items in trigger.
  We also check value returned from memory allocation functions.

mysql-test/r/trigger.result

@@ -150,6 +150,10 @@ create trigger trg before delete on t1 for each row set new.i:=1;
 ERROR HY000: There is no NEW row in on DELETE trigger
 create trigger trg after update on t1 for each row set new.i:=1;
 ERROR HY000: Updating of NEW row is not allowed in after trigger
+create trigger trg before update on t1 for each row set new.j:=1;
+ERROR 42S22: Unknown column 'j' in 'NEW'
+create trigger trg before update on t1 for each row set @a:=old.j;
+ERROR 42S22: Unknown column 'j' in 'OLD'
 create trigger trg before insert on t2 for each row set @a:=1;
 ERROR 42S02: Table 'test.t2' doesn't exist
 create trigger trg before insert on t1 for each row set @a:=1;

mysql-test/t/trigger.test

@@ -161,8 +161,10 @@ create trigger trg before update on t1 for each row set old.i:=1;
 create trigger trg before delete on t1 for each row set new.i:=1;
 --error 1362
 create trigger trg after update on t1 for each row set new.i:=1;
-# TODO: We should also test wrong field names here, we don't do it now
-#       because proper error handling is not in place yet.
+--error 1054
+create trigger trg before update on t1 for each row set new.j:=1;
+--error 1054
+create trigger trg before update on t1 for each row set @a:=old.j;
 
 
 #

sql/item.cc

@@ -3152,16 +3152,12 @@ void Item_insert_value::print(String *str)
   NOTE
     This function does almost the same as fix_fields() for Item_field
     but is invoked during trigger definition parsing and takes TABLE
-    object as its argument.
-
-  RETURN VALUES
-    0	 ok
-    1	 field was not found.
+    object as its argument. If proper field was not found in table
+    error will be reported at fix_fields() time.
 */
-bool Item_trigger_field::setup_field(THD *thd, TABLE *table,
+void Item_trigger_field::setup_field(THD *thd, TABLE *table,
                                      enum trg_event_type event)
 {
-  bool result= 1;
   uint field_idx= (uint)-1;
   bool save_set_query_id= thd->set_query_id;
 
@@ -3175,12 +3171,9 @@ bool Item_trigger_field::setup_field(THD *thd, TABLE *table,
     field= (row_version == OLD_ROW && event == TRG_EVENT_UPDATE) ?
              table->triggers->old_field[field_idx] :
              table->field[field_idx];
-    result= 0;
   }
 
   thd->set_query_id= save_set_query_id;
-
-  return result;
 }
 
 
@@ -3204,10 +3197,18 @@ bool Item_trigger_field::fix_fields(THD *thd,
     FIXME may be we still should bother about permissions here.
   */
   DBUG_ASSERT(fixed == 0);
+
+  if (field)
+  {
     // QQ: May be this should be moved to setup_field?
     set_field(field);
     fixed= 1;
     return 0;
+  }
+
+  my_error(ER_BAD_FIELD_ERROR, MYF(0), field_name,
+           (row_version == NEW_ROW) ? "NEW" : "OLD");
+  return 1;
 }
 
 

sql/item.h

@@ -1319,13 +1319,15 @@ class Item_trigger_field : public Item_field
   /* Is this item represents row from NEW or OLD row ? */
   enum row_version_type {OLD_ROW, NEW_ROW};
   row_version_type row_version;
+  /* Next in list of all Item_trigger_field's in trigger */
+  Item_trigger_field *next_trg_field;
 
   Item_trigger_field(row_version_type row_ver_par,
                      const char *field_name_par):
     Item_field((const char *)NULL, (const char *)NULL, field_name_par),
     row_version(row_ver_par)
   {}
-  bool setup_field(THD *thd, TABLE *table, enum trg_event_type event);
+  void setup_field(THD *thd, TABLE *table, enum trg_event_type event);
   enum Type type() const { return TRIGGER_FIELD_ITEM; }
   bool eq(const Item *item, bool binary_cmp) const;
   bool fix_fields(THD *, struct st_table_list *, Item **);

sql/mysql_priv.h

@@ -378,6 +378,15 @@ typedef struct st_sql_list {
     first= save->first;
     elements+= save->elements;
   }
+  inline void push_back(struct st_sql_list *save)
+  {
+    if (save->first)
+    {
+      *next= save->first;
+      next= save->next;
+      elements+= save->elements;
+    }
+  }
 } SQL_LIST;
 
 

sql/sp_head.cc

@@ -275,6 +275,11 @@ sp_head::init(LEX *lex)
   DBUG_ENTER("sp_head::init");
 
   lex->spcont= m_pcont= new sp_pcontext(NULL);
+  /*
+    Altough trg_table_fields list is used only in triggers we init for all
+    types of stored procedures to simplify reset_lex()/restore_lex() code.
+  */
+  lex->trg_table_fields.empty();
   my_init_dynamic_array(&m_instr, sizeof(sp_instr *), 16, 8);
   m_param_begin= m_param_end= m_returns_begin= m_returns_end= m_body_begin= 0;
   m_qname.str= m_db.str= m_name.str= m_params.str= m_retstr.str=
@@ -771,7 +776,7 @@ sp_head::reset_lex(THD *thd)
   sublex->spcont= oldlex->spcont;
   /* And trigger related stuff too */
   sublex->trg_chistics= oldlex->trg_chistics;
-  sublex->trg_table= oldlex->trg_table;
+  sublex->trg_table_fields.empty();
   sublex->sp_lex_in_use= FALSE;
   DBUG_VOID_RETURN;
 }
@@ -790,6 +795,7 @@ sp_head::restore_lex(THD *thd)
   // Update some state in the old one first
   oldlex->ptr= sublex->ptr;
   oldlex->next_state= sublex->next_state;
+  oldlex->trg_table_fields.push_back(&sublex->trg_table_fields);
 
   // Collect some data from the sub statement lex.
   sp_merge_funs(oldlex, sublex);

sql/sp_head.h

@@ -439,13 +439,9 @@ class sp_instr_set_trigger_field : public sp_instr
 
   virtual void print(String *str);
 
-  bool setup_field(THD *thd, TABLE *table, enum trg_event_type event)
-  {
-    return trigger_field.setup_field(thd, table, event);
-  }
-private:
-
   Item_trigger_field trigger_field;
+
+private:
   Item *value;
 }; // class sp_instr_trigger_field : public sp_instr
 

sql/sql_lex.cc

@@ -175,7 +175,6 @@ void lex_start(THD *thd, uchar *buf,uint length)
   lex->duplicates= DUP_ERROR;
   lex->sphead= NULL;
   lex->spcont= NULL;
-  lex->trg_table= NULL;
   lex->proc_list.first= 0;
 
   if (lex->spfuns.records)

sql/sql_lex.h

@@ -750,11 +750,13 @@ typedef struct st_lex
   /* Characterstics of trigger being created */
   st_trg_chistics trg_chistics;
   /*
-    Points to table being opened when we are parsing trigger definition
-    while opening table. 0 if we are parsing user provided CREATE TRIGGER
-    or any other statement. Used for NEW/OLD row field lookup in trigger.
+    List of all items (Item_trigger_field objects) representing fields in
+    old/new version of row in trigger. We use this list for checking whenever
+    all such fields are valid at trigger creation time and for binding these
+    fields to TABLE object at table open (altough for latter pointer to table
+    being opened is probably enough).
   */
-  TABLE *trg_table;
+  SQL_LIST trg_table_fields;
 
   st_lex() :result(0)
   {

sql/sql_parse.cc

@@ -3912,11 +3912,11 @@ mysql_execute_command(THD *thd)
     }
   case SQLCOM_CREATE_TRIGGER:
   {
-    /* We don't care much about trigger body at that point */
+    res= mysql_create_or_drop_trigger(thd, all_tables, 1);
+
+    /* We don't care about trigger body after this point */
     delete lex->sphead;
     lex->sphead= 0;
-
-    res= mysql_create_or_drop_trigger(thd, all_tables, 1);
     break;
   }
   case SQLCOM_DROP_TRIGGER:

sql/sql_trigger.cc

@@ -136,6 +136,7 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables)
   char dir_buff[FN_REFLEN], file_buff[FN_REFLEN];
   LEX_STRING dir, file;
   LEX_STRING *trg_def, *name;
+  Item_trigger_field *trg_field;
   List_iterator_fast<LEX_STRING> it(names_list);
 
   /* We don't allow creation of several triggers of the same type yet */
@@ -156,6 +157,31 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables)
     }
   }
 
+  /*
+    Let us check if all references to fields in old/new versions of row in
+    this trigger are ok.
+
+    NOTE: We do it here more from ease of use standpoint. We still have to
+    do some checks on each execution. E.g. we can catch privilege changes
+    only during execution. Also in near future, when we will allow access
+    to other tables from trigger we won't be able to catch changes in other
+    tables...
+
+    To simplify code a bit we have to create Fields for accessing to old row
+    values if we have ON UPDATE trigger.
+  */
+  if (!old_field && lex->trg_chistics.event == TRG_EVENT_UPDATE &&
+      prepare_old_row_accessors(table))
+    return 1;
+
+  for (trg_field= (Item_trigger_field *)(lex->trg_table_fields.first);
+       trg_field; trg_field= trg_field->next_trg_field)
+  {
+    trg_field->setup_field(thd, table, lex->trg_chistics.event);
+    if (trg_field->fix_fields(thd, (TABLE_LIST *)0, (Item **)0))
+      return 1;
+  }
+
   /*
     Here we are creating file with triggers and save all triggers in it.
     sql_create_definition_file() files handles renaming and backup of older
@@ -274,6 +300,44 @@ Table_triggers_list::~Table_triggers_list()
 }
 
 
+/*
+  Prepare array of Field objects which will represent OLD.* row values in
+  ON UPDATE trigger (by referencing to record[1] instead of record[0]).
+
+  SYNOPSIS
+    prepare_old_row_accessors()
+      table - pointer to TABLE object for which we are creating fields.
+
+  RETURN VALUE
+    False - success
+    True  - error
+*/
+bool Table_triggers_list::prepare_old_row_accessors(TABLE *table)
+{
+  Field **fld, **old_fld;
+
+  if (!(old_field= (Field **)alloc_root(&table->mem_root,
+                                        (table->fields + 1) *
+                                        sizeof(Field*))))
+    return 1;
+
+  for (fld= table->field, old_fld= old_field; *fld; fld++, old_fld++)
+  {
+    /*
+      QQ: it is supposed that it is ok to use this function for field
+      cloning...
+    */
+    if (!(*old_fld= (*fld)->new_field(&table->mem_root, table)))
+      return 1;
+    (*old_fld)->move_field((my_ptrdiff_t)(table->record[1] -
+                                          table->record[0]));
+  }
+  *old_fld= 0;
+
+  return 0;
+}
+
+
 /*
   Check whenever .TRG file for table exist and load all triggers it contains.
 
@@ -317,7 +381,6 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
     if (!strncmp(triggers_file_type.str, parser->type()->str,
                  parser->type()->length))
     {
-      Field **fld, **old_fld;
       Table_triggers_list *triggers=
         new (&table->mem_root) Table_triggers_list();
 
@@ -330,30 +393,9 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
 
       table->triggers= triggers;
 
-      /*
-        We have to prepare array of Field objects which will represent OLD.*
-        row values by referencing to record[1] instead of record[0]
-
-        TODO: This could be avoided if there is no ON UPDATE trigger.
-      */
-      if (!(triggers->old_field=
-              (Field **)alloc_root(&table->mem_root, (table->fields + 1) *
-                                                     sizeof(Field*))))
-        DBUG_RETURN(1);
-
-      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 (!(*old_fld= (*fld)->new_field(&table->mem_root, table)))
+      /* TODO: This could be avoided if there is no ON UPDATE trigger. */
+      if (triggers->prepare_old_row_accessors(table))
         DBUG_RETURN(1);
-        (*old_fld)->move_field((my_ptrdiff_t)(table->record[1] -
-                                              table->record[0]));
-      }
-      *old_fld= 0;
 
       List_iterator_fast<LEX_STRING> it(triggers->definitions_list);
       LEX_STRING *trg_create_str, *trg_name_str;
@@ -365,7 +407,7 @@ 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);
-        lex.trg_table= table;
+
         if (yyparse((void *)thd) || thd->is_fatal_error)
         {
           /*
@@ -400,6 +442,21 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
         if (triggers->names_list.push_back(trg_name_str, &table->mem_root))
           goto err_with_lex_cleanup;
 
+        /*
+          Let us bind Item_trigger_field objects representing access to fields
+          in old/new versions of row in trigger to Field objects in table being
+          opened.
+
+          We ignore errors here, because if even something is wrong we still will
+          be willing to open table to perform some operations (e.g. SELECT)...
+          Anyway some things can be checked only during trigger execution.
+        */
+        for (Item_trigger_field *trg_field=
+               (Item_trigger_field *)(lex.trg_table_fields.first);
+             trg_field;
+             trg_field= trg_field->next_trg_field)
+          trg_field->setup_field(thd, table, lex.trg_chistics.event);
+
         lex_end(&lex);
       }
       thd->lex= old_lex;

sql/sql_trigger.h

@@ -65,4 +65,7 @@ class Table_triggers_list: public Sql_alloc
   }
 
   friend class Item_trigger_field;
+
+private:
+  bool prepare_old_row_accessors(TABLE *table);
 };

sql/sql_yacc.yy

@@ -1239,7 +1239,8 @@ create:
               YYABORT;
             }
 
-            sp= new sp_head();
+            if (!(sp= new sp_head()))
+              YYABORT;
             sp->reset_thd_mem_root(YYTHD);
             sp->init(lex);
             
@@ -6622,6 +6623,7 @@ simple_ident_q:
               (!my_strcasecmp(system_charset_info, $1.str, "NEW") || 
                !my_strcasecmp(system_charset_info, $1.str, "OLD")))
           {
+            Item_trigger_field *trg_fld;
             bool new_row= ($1.str[0]=='N' || $1.str[0]=='n');
             
             if (lex->trg_chistics.event == TRG_EVENT_INSERT &&
@@ -6638,23 +6640,18 @@ simple_ident_q:
               YYABORT;
             }
             
-            Item_trigger_field *trg_fld= 
-              new Item_trigger_field(new_row ? Item_trigger_field::NEW_ROW :
+            if (!(trg_fld= new Item_trigger_field(new_row ?
+                                                  Item_trigger_field::NEW_ROW:
                                                   Item_trigger_field::OLD_ROW,
-                                               $3.str);
+                                                  $3.str)))
+              YYABORT;
           
-            if (lex->trg_table &&
-                trg_fld->setup_field(thd, lex->trg_table,
-                                     lex->trg_chistics.event))
-            {
             /*
-                FIXME. Far from perfect solution. See comment for 
-                "SET NEW.field_name:=..." for more info.
+              Let us add this item to list of all Item_trigger_field objects
+              in trigger.
             */
-              my_error(ER_BAD_FIELD_ERROR, MYF(0),
-                       $3.str, new_row ? "NEW": "OLD");
-              YYABORT;
-            }
+            lex->trg_table_fields.link_in_list((byte *)trg_fld,
+              (byte**)&trg_fld->next_trg_field);
             
             $$= (Item *)trg_fld;
           }
@@ -7156,28 +7153,19 @@ option_value:
                 /* QQ: Shouldn't this be field's default value ? */
                 it= new Item_null();
               }
-              i= new sp_instr_set_trigger_field(lex->sphead->instructions(),
-                                                lex->spcont, $1.base_name, it);
-              if (lex->trg_table && i->setup_field(YYTHD, lex->trg_table,
-                                                   lex->trg_chistics.event))
-              {
+              
+              if (!(i= new sp_instr_set_trigger_field(
+                             lex->sphead->instructions(), lex->spcont,
+                             $1.base_name, it)))
+                YYABORT;
+              
               /*
-                  FIXME. Now we are catching this kind of errors only
-                  during opening tables. But this doesn't save us from most
-                  common user error - misspelling field name, because we
-                  will bark too late in this case... Moreover it is easy to
-                  make table unusable with such kind of error...
-
-                  So in future we either have to parse trigger definition
-                  second time during create trigger or gather all trigger
-                  fields in one list and perform setup_field() for them as
-                  separate stage.
-
-                  Error message also should be improved.
+                Let us add this item to list of all Item_trigger_field
+                objects in trigger.
               */
-                my_error(ER_BAD_FIELD_ERROR, MYF(0), $1.base_name, "NEW");
-                YYABORT;
-              }
+              lex->trg_table_fields.link_in_list((byte *)&i->trigger_field,
+                     (byte **)&i->trigger_field.next_trg_field);
+
               lex->sphead->add_instr(i);
             }
             else if ($1.var)