
Hello Monty. I checked your patch for MDEV-6112 multiple triggers per table. Here are some quick notes. I'll send a detailed review separately. 1. It would be nice to push refactoring changes in separate patch: a. do sql_mode_t refactoring b. "trg_idx -> Trigger *" refactoring c. opt_debugging related things 2. Why not to reuse the FOLLOWING_SYM and PRECEDING_SYM keywords instead of adding new ones FOLLOWS_SYM, PRECEDES_SYM ? (this is a non-standard syntax anyway) 3. I'd prefer PRECEDES/FOLLOWS to go before FOR EACH ROW. "FOR EACH ROW stmt" is a kind of single clause responsible for the action. It's very confusing to have PRECEDES/FOLLOWS characteristics in between "FOR EACH ROW" and stmt. 4. You have a lot of changes related to trim_whitespace() where you declare an unused "prefix_removed" variable. It would be nice to avoid declaring it. Possible ways: a. Perhaps the new function version can be defined like this: size_t trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str); and make it return "prefix length removed", instead of adding a new parameter. b. Another option is just to overload it: extern void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str, uint *prefix_removed); inline void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str) { uint not_used; trim_whitespace(cs, str, ¬_used); } 5. The "CREATED" field is not populated well. I'm getting strange values like 1970-04-24 09:35:00.30. Btw, his query returns a correct value: SELECT FROM_UNIXTIME(UNIX_TIMESTAMP(CREATED)*100) FROM INFORMATION_SCHEMA.triggers; You must have forgotten to multiply or delete to 100 somewhere. 6. In replication tests it's better to make sure that CREATED is safely replicated (instead of changing it to #). By the way, in other tests it's also a good idea not to use # for CREATED. (see the previous problem) You can use this: SET TIMESTAMP=UNIX_TIMESTAMP('2001-01-01 10:20:30'); ... SET TIMESTAM=DEFAULT; 7. I suggest to move this code into a method, say find_trigger(): + if (table->triggers) + { + Trigger *t; + for (t= table->triggers->get_trigger(TRG_EVENT_INSERT, + TRG_ACTION_BEFORE); + t; + t= t->next) + if (t->is_fields_updated_in_trigger(&full_part_field_set)) + DBUG_RETURN(false); + } It's repeated at least two times. 8. The following structure fields are defined two times: enum trigger_order_type ordering_clause; LEX_STRING anchor_trigger_name; The first time for st_trg_chistics. The second time for %union in sql_yacc.yy Note, there will be the third time in sql_yacc_ora.yy. Please define a structure in sql_lex.h instead: struct st_trg_execution_order_chistics { /** FOLLOWS or PRECEDES as specified in the CREATE TRIGGER statement. */ enum trigger_order_type ordering_clause; /** Trigger name referenced in the FOLLOWS/PRECEDES clause of the CREATE TRIGGER statement. */ LEX_STRING anchor_trigger_name; }; and reuse it in here: struct st_trg_chirstics: public st_trg_execution_order_chistics { ... }; and in here: %union { ... st_trg_execition_order_chistics trg_execution_order_chirstics; ... }; Please also rename trg_characteristics to trg_execution_order_chistics, because "trg_characteristics" is more suitable to something having type st_trg_chirstics rather than st_trg_execution_order_chistics. Greetings.