maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #09980
Re: Review for MDEV-6112 multiple triggers per table
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());
Why this change? Is it related to the MDEV itself?
<cut>
> @@ -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
trigger.
> -
> + @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 -
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.
References
