Hello Monty, This is a more detailed review for MDEV-6112.

--- a/mysql-test/t/trigger-compat.test +++ b/mysql-test/t/trigger-compat.test

<cut>

+--replace_column 17 # SELECT trigger_name, definer FROM INFORMATION_SCHEMA.TRIGGERS ORDER BY trigger_name;

The above replace_column is not needed.

--- a/sql/sql_show.cc +++ b/sql/sql_show.cc <cut> @@ -6385,43 +6399,59 @@ static int get_schema_constraints_record(THD *thd, TABLE_LIST *tables, }

-static bool store_trigger(THD *thd, TABLE *table, LEX_STRING *db_name, - LEX_STRING *table_name, LEX_STRING *trigger_name, +static bool store_trigger(THD *thd, Trigger *trigger, + TABLE *table, LEX_STRING *db_name, + LEX_STRING *table_name, enum trg_event_type event, - enum trg_action_time_type timing, - LEX_STRING *trigger_stmt, - ulong sql_mode, - LEX_STRING *definer_buffer, - LEX_STRING *client_cs_name, - LEX_STRING *connection_cl_name, - LEX_STRING *db_cl_name) + enum trg_action_time_type timing) { CHARSET_INFO *cs= system_charset_info; LEX_STRING sql_mode_rep; + MYSQL_TIME timestamp; + char definer_holder[USER_HOST_BUFF_SIZE]; + LEX_STRING definer_buffer, trigger_stmt, trigger_body; + definer_buffer.str= definer_holder; + + trigger->get_trigger_info(&trigger_stmt, &trigger_body, &definer_buffer);

restore_record(table, s->default_values); table->field[0]->store(STRING_WITH_LEN("def"), cs); table->field[1]->store(db_name->str, db_name->length, cs); - table->field[2]->store(trigger_name->str, trigger_name->length, cs); + table->field[2]->store(trigger->name.str, trigger->name.length, cs); table->field[3]->store(trg_event_type_names[event].str, trg_event_type_names[event].length, cs); table->field[4]->store(STRING_WITH_LEN("def"), cs); table->field[5]->store(db_name->str, db_name->length, cs); table->field[6]->store(table_name->str, table_name->length, cs); - table->field[9]->store(trigger_stmt->str, trigger_stmt->length, cs); + table->field[7]->set_notnull();

Why is set_notnull() needed? I_S.TRIGGERS.ACTION_ORDER is declared as NOT NULL.

+ table->field[7]->store(trigger->action_order); + table->field[9]->store(trigger_body.str, trigger_body.length, cs); table->field[10]->store(STRING_WITH_LEN("ROW"), cs); table->field[11]->store(trg_action_time_type_names[timing].str, trg_action_time_type_names[timing].length, cs); table->field[14]->store(STRING_WITH_LEN("OLD"), cs); table->field[15]->store(STRING_WITH_LEN("NEW"), cs);

- sql_mode_string_representation(thd, sql_mode, &sql_mode_rep); + if (trigger->create_time) + { + table->field[16]->set_notnull();

You use set_notnull() here, but don't use it in show_create_trigger_impl. See below. <cut>

@@ -9307,35 +9322,45 @@ int finalize_schema_table(st_plugin_int *plugin) DBUG_RETURN(0); }

+/* + This is used to create a timestamp field +*/ + +MYSQL_TIME zero_time={ 0,0,0,0,0,0,0,0, MYSQL_TIMESTAMP_TIME }; + +class Item_timestamp_literal: public Item_datetime_literal +{ +public: + Item_timestamp_literal(THD *thd, uint decimals): + Item_datetime_literal(thd, &zero_time, decimals) + {} + enum_field_types field_type() const { return MYSQL_TYPE_TIMESTAMP; } +}; +

Why you do need MYSQL_TYPE_TIMESTAMP here? "CREATED" in triggers_fields_info is defined as MYSQL_TYPE_DATETIME. "mysql --column-type-info" reports MYSQL_TYPE_DATETIME for "CREATED". I tried to replace Item_timestamp_literal to Item_datetime_literal and it seems to work fine. Please use Item_datetime_literal.

/** Output trigger information (SHOW CREATE TRIGGER) to the client.

@param thd Thread context. - @param triggers List of triggers for the table. - @param trigger_idx Index of the trigger to dump. + @param trigger Trigger to dump

@return Operation status @retval TRUE Error. @retval FALSE Success. */

-static bool show_create_trigger_impl(THD *thd, - Table_triggers_list *triggers, - int trigger_idx) +static bool show_create_trigger_impl(THD *thd, Trigger *trigger) { int ret_code; Protocol *p= thd->protocol; List<Item> fields; - LEX_STRING trg_name; - ulonglong trg_sql_mode; - LEX_STRING trg_sql_mode_str; + LEX_STRING trg_sql_mode_str, trg_body; LEX_STRING trg_sql_original_stmt; - LEX_STRING trg_client_cs_name; - LEX_STRING trg_connection_cl_name; - LEX_STRING trg_db_cl_name; + LEX_STRING trg_definer; CHARSET_INFO *trg_client_cs; MEM_ROOT *mem_root= thd->mem_root; + char definer_holder[USER_HOST_BUFF_SIZE]; + trg_definer.str= definer_holder;

/* TODO: Check privileges here. This functionality will be added by @@ -9349,20 +9374,12 @@ static bool show_create_trigger_impl(THD *thd,

/* Prepare trigger "object". */

- triggers->get_trigger_info(thd, - trigger_idx, - &trg_name, - &trg_sql_mode, - &trg_sql_original_stmt, - &trg_client_cs_name, - &trg_connection_cl_name, - &trg_db_cl_name); - - sql_mode_string_representation(thd, trg_sql_mode, &trg_sql_mode_str); + trigger->get_trigger_info(&trg_sql_original_stmt, &trg_body,

&trg_definer);

+ sql_mode_string_representation(thd, trigger->sql_mode, &trg_sql_mode_str);

/* Resolve trigger client character set. */

- if (resolve_charset(trg_client_cs_name.str, NULL, &trg_client_cs)) + if (resolve_charset(trigger->client_cs_name.str, NULL, &trg_client_cs)) return TRUE;

/* Send header. */ @@ -9425,18 +9446,30 @@ static bool show_create_trigger_impl(THD *thd,

<cut>

+ if (trigger->create_time) + { + MYSQL_TIME timestamp; + thd->variables.time_zone->gmt_sec_to_TIME(×tamp, + trigger->create_time/100); + timestamp.second_part= (trigger->create_time % 100) * 10000; + p->store(×tamp, 2);

Not set_notnull() here, but there is set_notnull() in the same place in store_trigger(). It seems both places should be identical.

+ } + else + p->store_null(); + + ret_code= p->write();

if (!ret_code) <cut>

--- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -34,30 +34,17 @@ #include "sp_cache.h" // sp_invalidate_cache #include <mysys_err.h>

-/*************************************************************************/

- -template <class T> -inline T *alloc_type(MEM_ROOT *m) -{ - return (T *) alloc_root(m, sizeof (T)); -} - -/* - NOTE: Since alloc_type() is declared as inline, alloc_root() calls should - be inlined by the compiler. So, implementation of alloc_root() is not - needed. However, let's put the implementation in object file just in case - of stupid MS or other old compilers. -*/ - -template LEX_STRING *alloc_type<LEX_STRING>(MEM_ROOT *m); -template ulonglong *alloc_type<ulonglong>(MEM_ROOT *m); - -inline LEX_STRING *alloc_lex_string(MEM_ROOT *m) +LEX_STRING *make_lex_string(LEX_STRING *lex_str, const char* str, uint length, + MEM_ROOT *mem_root) { - return alloc_type<LEX_STRING>(m); + if (!(lex_str->str= strmake_root(mem_root, str, length))) + return 0; + lex_str->length= length; + return lex_str;

This make_lex_string() should probably go into some *.h file, and a very similar THD::make_lex_string() defined in sql_class.h should reuse it. Exactly the same duplicate code. <cut>

@@ -220,6 +205,11 @@ static File_option triggers_file_parameters[]= my_offsetof(class Table_triggers_list, db_cl_names), FILE_OPTIONS_STRLIST }, + { + { C_STRING_WITH_LEN("created") }, + my_offsetof(class Table_triggers_list, create_times), + FILE_OPTIONS_ULLLIST + }, { { 0, 0 }, 0, FILE_OPTIONS_STRING } };

"created" is stored in t1.TRG as (seconds*100+frac). This is not future-proof. If we ever want to change it to store more digits, we'll be in troubles. Can we instead of this format: created=147504759488 use a better one, with a dot followed by the fractional part: created=1475047594.88 <cut>

@@ -587,7 +612,7 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) end: if (!result) { - result= write_bin_log(thd, TRUE, stmt_query.ptr(), stmt_query.length()); + result= write_bin_log(thd, TRUE, stmt_query.c_ptr(), stmt_query.length());

@@ -609,8 +634,8 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) }

/** - Build stmt_query to write it in the bin-log - and get the trigger definer. + Build stmt_query to write it in the bin-log, the statement to write in + the trigger file and the trigger definer.

@param thd current thread context (including trigger definition in LEX) @@ -618,7 +643,8 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) trigger is created. @param[out] stmt_query after successful return, this string contains well-formed statement for creation this

- + @param[out] trigger_def query to be stored in trigger file. As stmt_query, + but without "OR REPLACE" and no FOLLOWS/PRECEDES. @param[out] trg_definer The triggger definer. @param[out] trg_definer_holder Used as a buffer for definer.

@@ -630,12 +656,16 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) simultaneously NULL-strings (non-SUID/old trigger) or valid strings (SUID/new trigger). */ + static void build_trig_stmt_query(THD *thd, TABLE_LIST *tables, - String *stmt_query, + String *stmt_query, String *trigger_def, LEX_STRING *trg_definer, char trg_definer_holder[]) { + LEX_STRING stmt_definition; LEX *lex= thd->lex; + uint prefix_trimmed, suffix_trimmed; + size_t original_length;

/* Create a query with the full trigger definition. @@ -651,18 +683,42 @@ static void build_trig_stmt_query(THD *thd, TABLE_LIST *tables, /* SUID trigger */ lex->definer->set_lex_string(trg_definer, trg_definer_holder); append_definer(thd, stmt_query, &lex->definer->user, &lex->definer->host); + append_definer(thd, trigger_def, &lex->definer->user, &lex->definer->host); } else { *trg_definer= empty_lex_str; }

- LEX_STRING stmt_definition; - stmt_definition.str= (char*) thd->lex->stmt_definition_begin; - stmt_definition.length= thd->lex->stmt_definition_end - - thd->lex->stmt_definition_begin; - trim_whitespace(thd->charset(), &stmt_definition); + + /* Create statement for binary logging */ + stmt_definition.str= (char*) lex->stmt_definition_begin; + stmt_definition.length= (lex->stmt_definition_end - + lex->stmt_definition_begin); + original_length= stmt_definition.length; + trim_whitespace(thd->charset(), &stmt_definition, &prefix_trimmed); + suffix_trimmed= original_length - stmt_definition.length -

Why this change? Is it related to the MDEV itself? <cut> trigger. prefix_trimmed;

+ stmt_query->append(stmt_definition.str, stmt_definition.length); + + /* Create statement for storing trigger (without trigger order) */ + if (lex->trg_chistics.ordering_clause == TRG_ORDER_NONE) + trigger_def->append(stmt_definition.str, stmt_definition.length); + else + { + /* Copy data before FOLLOWS/PRECEDES trigger_name */ + trigger_def->append(stmt_definition.str, + (lex->trg_chistics.ordering_clause_begin - + lex->stmt_definition_begin) - prefix_trimmed); + /* Copy data after FOLLOWS/PRECEDES trigger_name */ + trigger_def->append(stmt_definition.str + + (lex->trg_chistics.ordering_clause_end - + lex->stmt_definition_begin) + - prefix_trimmed, + (lex->stmt_definition_end - + lex->trg_chistics.ordering_clause_end) - + suffix_trimmed); + } }

The above does not seem to be covered in the tests. There should be two tests: - in /suite/binlog/, to make sure that the query contains the FOLLOWS/PRECEDES part. - in /suite/rpl/, to make sure that ACTION_ORDER is correctly reproduced on the slave side. <cut>

@@ -860,34 +887,91 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables,

+ lex_string_set(&trigger->connection_cl_name, thd->variables.collation_connection->name); - - lex_string_set(trg_db_cl_name, + lex_string_set(&trigger->db_cl_name, get_default_db_collation(thd, tables->db)->name);

- build_trig_stmt_query(thd, tables, stmt_query, - trg_definer, trg_definer_holder); - - trg_def->str= stmt_query->c_ptr_safe(); - trg_def->length= stmt_query->length(); + /* Add trigger in it's correct place */ + if (add_trigger(lex->trg_chistics.event, + lex->trg_chistics.action_time, + lex->trg_chistics.ordering_clause, + &lex->trg_chistics.anchor_trigger_name, + trigger)) + goto err_with_cleanup;

- /* Create trigger definition file. */ + /* Create trigger definition file .TRG */ + if (create_lists_needed_for_files(thd->mem_root)) + goto err_with_cleanup;

if (!sql_create_definition_file(NULL, &file, &triggers_file_type, (uchar*)this, triggers_file_parameters)) - return false; + DBUG_RETURN(false);

Looks like a memory leak. Isn't a "delete trigger" needed? <cut>

+Trigger *Table_triggers_list::find_trigger(const LEX_STRING *name, + bool remove_from_list) +{ + for (uint i= 0; i < (uint)TRG_EVENT_MAX; i++) + { + for (uint j= 0; j < (uint)TRG_ACTION_MAX; j++) + { + Trigger **parent, *trigger; + + parent= &triggers[i][j]; + for (parent= &triggers[i][j];

Looks like "parent= &triggers[i][j]" is done two times.

@@ -979,85 +1104,70 @@ static bool save_trigger_file(Table_triggers_list *triggers, const char *db, @retval True error */ + bool Table_triggers_list::drop_trigger(THD *thd, TABLE_LIST *tables, String *stmt_query) { - const char *sp_name= thd->lex->spname->m_name.str; // alias - - LEX_STRING *name; + const LEX_STRING *sp_name= &thd->lex->spname->m_name; // alias char path[FN_REFLEN]; + Trigger *trigger;

- List_iterator_fast<LEX_STRING> it_name(names_list); - - List_iterator<ulonglong> it_mod(definition_modes_list); - List_iterator<LEX_STRING> it_def(definitions_list); - List_iterator<LEX_STRING> it_definer(definers_list); - List_iterator<LEX_STRING> it_client_cs_name(client_cs_names); - List_iterator<LEX_STRING> it_connection_cl_name(connection_cl_names); - List_iterator<LEX_STRING> it_db_cl_name(db_cl_names); + stmt_query->set(thd->query(), thd->query_length(), stmt_query->charset());

- stmt_query->append(thd->query(), thd->query_length()); - - while ((name= it_name++)) + /* Find and delete trigger from list */ + if (!(trigger= find_trigger(sp_name, true))) { - it_def++; - it_mod++; - it_definer++; - it_client_cs_name++; - it_connection_cl_name++; - it_db_cl_name++; - - if (my_strcasecmp(table_alias_charset, sp_name, name->str) == 0) - { - /* - Again we don't care much about other things required for - clean trigger removing since table will be reopened anyway. - */ - it_def.remove(); - it_mod.remove(); - it_definer.remove(); - it_client_cs_name.remove(); - it_connection_cl_name.remove(); - it_db_cl_name.remove(); - - if (definitions_list.is_empty()) - { - /* - TODO: Probably instead of removing .TRG file we should move - to archive directory but this should be done as part of - parse_file.cc functionality (because we will need it - elsewhere). - */ - if (rm_trigger_file(path, tables->db, tables->table_name)) - return 1; - } - else - { - if (save_trigger_file(this, tables->db, tables->table_name)) - return 1; - } + my_message(ER_TRG_DOES_NOT_EXIST, ER_THD(thd, ER_TRG_DOES_NOT_EXIST), + MYF(0)); + return 1; + }

- if (rm_trigname_file(path, tables->db, sp_name)) - return 1; - return 0; - } + if (!count) // If no more triggers + { + /* + TODO: Probably instead of removing .TRG file we should move + to archive directory but this should be done as part of + parse_file.cc functionality (because we will need it + elsewhere). + */ + if (rm_trigger_file(path, tables->db, tables->table_name)) + return 1; } + else + { + if (save_trigger_file(thd, tables->db, tables->table_name)) + return 1; + } + + if (rm_trigname_file(path, tables->db, sp_name->str)) + return 1;

Can you please clarify why these two files are deleted: tables->table_name and sp_name->table_name ? <cut>

/** + Add trigger in the correct position according to ordering clause + Also update action order + + If anchor_trigger doesn't exist, add it last +*/ + +bool Table_triggers_list::add_trigger(trg_event_type event, + trg_action_time_type action_time, + trigger_order_type ordering_clause, + LEX_STRING *anchor_trigger_name, + Trigger *trigger)

st_trg_execution_order can be passed here, instead of separate ordering_clause and anchor_trigger_name. <cut>

--- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy <cut> @@ -16494,14 +16531,16 @@ trigger_tail: lex->ident.str= $9; lex->ident.length= $13 - $9; lex->spname= $5; + (*static_cast<st_trg_execution_order*>(&lex->trg_chistics))= ($18); + lex->trg_chistics.ordering_clause_end= lip->get_cpp_ptr();

if (!make_sp_head(thd, $5, TYPE_ENUM_TRIGGER)) MYSQL_YYABORT;

- lex->sphead->set_body_start(thd, lip->get_cpp_ptr()); + lex->sphead->set_body_start(thd, lip->get_cpp_tok_start());

Can you clarify please why this change ^^^^ ? On 09/27/2016 04:33 PM, Alexander Barkov wrote:

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.