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

@@ -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 @@ public:
   /* 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 @@ public:
 
   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 @@ create_error:
     }
   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 @@ public:
   }
 
   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)