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.