diff --git a/mysql-test/r/trigger.result b/mysql-test/r/trigger.result index b45aaea0cbe..4a85097cfce 100644 --- a/mysql-test/r/trigger.result +++ b/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; diff --git a/mysql-test/t/trigger.test b/mysql-test/t/trigger.test index 7dc976cf716..d4879b22bae 100644 --- a/mysql-test/t/trigger.test +++ b/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; # diff --git a/sql/item.cc b/sql/item.cc index 944d377282d..6ebe34f91dc 100644 --- a/sql/item.cc +++ b/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); - // QQ: May be this should be moved to setup_field? - set_field(field); - fixed= 1; - return 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; } diff --git a/sql/item.h b/sql/item.h index 23515abc7a8..933233e38c3 100644 --- a/sql/item.h +++ b/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 **); diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index eced813f75d..f948bf13226 100644 --- a/sql/mysql_priv.h +++ b/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; diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 7db79128bb8..114ff0d451a 100644 --- a/sql/sp_head.cc +++ b/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); diff --git a/sql/sp_head.h b/sql/sp_head.h index 4bfe1076f65..c4d2068661c 100644 --- a/sql/sp_head.h +++ b/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 diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 1cbe004caa0..971ba125859 100644 --- a/sql/sql_lex.cc +++ b/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) diff --git a/sql/sql_lex.h b/sql/sql_lex.h index ce22caa13fc..3ae6f91bdd6 100644 --- a/sql/sql_lex.h +++ b/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) { diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 8f4e922416b..89807765421 100644 --- a/sql/sql_parse.cc +++ b/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: diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc index 7637679430f..a88dc0b20bf 100644 --- a/sql/sql_trigger.cc +++ b/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 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,31 +393,10 @@ 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*)))) + /* TODO: This could be avoided if there is no ON UPDATE trigger. */ + if (triggers->prepare_old_row_accessors(table)) 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))) - DBUG_RETURN(1); - (*old_fld)->move_field((my_ptrdiff_t)(table->record[1] - - table->record[0])); - } - *old_fld= 0; - List_iterator_fast it(triggers->definitions_list); LEX_STRING *trg_create_str, *trg_name_str; char *trg_name_buff; @@ -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; diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h index d0376f056d9..82e7c1ce023 100644 --- a/sql/sql_trigger.h +++ b/sql/sql_trigger.h @@ -65,4 +65,7 @@ public: } friend class Item_trigger_field; + +private: + bool prepare_old_row_accessors(TABLE *table); }; diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 14086514ff7..6cb02fa4046 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1238,8 +1238,9 @@ create: my_error(ER_SP_NO_RECURSIVE_CREATE, MYF(0), "TRIGGER"); 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 : - Item_trigger_field::OLD_ROW, - $3.str); - - 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. - */ - my_error(ER_BAD_FIELD_ERROR, MYF(0), - $3.str, new_row ? "NEW": "OLD"); + if (!(trg_fld= new Item_trigger_field(new_row ? + Item_trigger_field::NEW_ROW: + Item_trigger_field::OLD_ROW, + $3.str))) YYABORT; - } + + /* + Let us add this item to list of all Item_trigger_field objects + in trigger. + */ + 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)) - { - /* - 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. - */ - my_error(ER_BAD_FIELD_ERROR, MYF(0), $1.base_name, "NEW"); + + if (!(i= new sp_instr_set_trigger_field( + lex->sphead->instructions(), lex->spcont, + $1.base_name, it))) YYABORT; - } + + /* + Let us add this item to list of all Item_trigger_field + objects in trigger. + */ + 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)