[Maria-developers] MDEV-7286 TRIGGER: CREATE OR REPLACE, CREATE IF NOT EXISTS
As regards TRIGGERS in MySQL I am very much missing what PostgreSQL (and maybe other RDBMS as well) has: an option to [temporarily] disable TRIGGGER[S] for all or specific users - like listed in PostgreSQL docs: ( http://www.postgresql.org/docs/9.2/static/sql-altertable.html) ALTER TABLE .. DISABLE TRIGGER [ trigger_name | ALL | USER ] ENABLE TRIGGER [ trigger_name | ALL | USER ] .. A TRIGGER may work fine with the application it was designed to be used with. But you may need to do some manual maintenance and then it is not always desirable that TRIGGERS will fire. It can actually start a chain of unwanted changes that can be very dificult to correct (unless you resort to the brutal solution of taking the database offline, dropping TRIGGERS, do the maintenance and recreate the TRIGGERS). (unrelated to the code review requested, of course). -- Peter On Mon, Feb 9, 2015 at 1:44 PM, Alexander Barkov <bar@mariadb.org> wrote:
Hi Serg,
Please review a patch for MDEV-7286.
Thanks.
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi Peter, On Mon, Feb 9, 2015 at 9:26 AM, Peter Laursen <peter_laursen@webyog.com> wrote:
As regards TRIGGERS in MySQL I am very much missing what PostgreSQL (and maybe other RDBMS as well) has: an option to [temporarily] disable TRIGGGER[S] for all or specific users - like listed in PostgreSQL docs: (http://www.postgresql.org/docs/9.2/static/sql-altertable.html)
ALTER TABLE .. DISABLE TRIGGER [ trigger_name | ALL | USER ] ENABLE TRIGGER [ trigger_name | ALL | USER ] ..
A TRIGGER may work fine with the application it was designed to be used with. But you may need to do some manual maintenance and then it is not always desirable that TRIGGERS will fire. It can actually start a chain of unwanted changes that can be very dificult to correct (unless you resort to the brutal solution of taking the database offline, dropping TRIGGERS, do the maintenance and recreate the TRIGGERS).
I agree that MySQL and MariaDB could benefit from some method to disable triggers for a particular session or user. However, I think you may have slightly misunderstood PostgreSQL's documentation. Going over the usage of these commands in PostgreSQL: ALTER TABLE foo DISABLE TRIGGER USER; The above command doesn't disable all triggers on the table for a particular user. It disables all user-created triggers on the table for all users. ALTER TABLE foo DISABLE TRIGGER ALL; The above command disables all triggers on the table for all users, including triggers generated by the system to enforce things like check constraints, foreign key constraints, etc. Anyway, I submitted a feature request on the JIRA for a feature like this. Feel free to vote for the issue or to add comments to the issue about what you would like this feature to look like. https://mariadb.atlassian.net/browse/MDEV-7579 Geoff
Hi, Alexander!
Please review a patch for MDEV-7286.
Pretty good. I have just a couple of comments about build_trig_stmt_query() usage, see below.
diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc index 4b20813..d3713cc 100644 --- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -608,6 +608,67 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create) DBUG_RETURN(result); }
+/** + Build stmt_query to write it in the bin-log + and get the trigger definer. + + @param thd current thread context (including trigger definition in + LEX) + @param tables table list containing one open table for which the + trigger is created. + @param[out] stmt_query after successful return, this string contains + well-formed statement for creation this trigger. + + @param[out] trg_definer The triggger definer. + @param[out] trg_definer_holder Used as a buffer for definer. + + @note + - Assumes that trigger name is fully qualified. + - NULL-string means the following LEX_STRING instance: + { str = 0; length = 0 }. + - In other words, definer_user and definer_host should contain + 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, + LEX_STRING *trg_definer, + char trg_definer_holder[]) +{ + LEX *lex= thd->lex; + + /* + Create well-formed trigger definition query. Original query is not + appropriated, because definer-clause can be not truncated.
you don't have to preserve old comments verbatim, feel free to fix them as you see fit. E.g. here I'd say "is not appropriate" or, better, rewrite the whole comment to make sense (what does it mean "definer-clause can be not truncated"?)
+ */ + stmt_query->append(STRING_WITH_LEN("CREATE ")); + + if (lex->create_info.or_replace()) + stmt_query->append(STRING_WITH_LEN("OR REPLACE ")); + + if (lex->sphead->m_chistics->suid != SP_IS_NOT_SUID) + { + /* SUID trigger */ + lex->definer->set_lex_string(trg_definer, trg_definer_holder); + /* + Append definer-clause if the trigger is SUID (a usual trigger in + new MySQL versions).
And this comment I'd remove completely. "new MySQL versions" isn't true anymore. And what's left is "Append definer" one line before a function append_definer() is called. Useless.
+ */ + append_definer(thd, stmt_query, &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); + stmt_query->append(stmt_definition.str, stmt_definition.length); +} +
/** Create trigger for table. @@ -722,8 +794,29 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, /* Use the filesystem to enforce trigger namespace constraints. */ if (!access(trigname_buff, F_OK)) { - my_error(ER_TRG_ALREADY_EXISTS, MYF(0)); - return 1; + if (lex->create_info.or_replace()) + { + String drop_trg_query; + drop_trg_query.append("DROP TRIGGER "); + drop_trg_query.append(lex->spname->m_name.str); + if (drop_trigger(thd, tables, &drop_trg_query)) + return 1; + } + else if (lex->create_info.if_not_exists()) + { + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, + ER_TRG_ALREADY_EXISTS, ER(ER_TRG_ALREADY_EXISTS), + trigname_buff); + LEX_STRING trg_definer_tmp; + build_trig_stmt_query(thd, tables, stmt_query, + &trg_definer_tmp, trg_definer_holder);
Why?
+ return false; + } + else + { + my_error(ER_TRG_ALREADY_EXISTS, MYF(0)); + return true; + } }
trigname.trigger_table.str= tables->table_name; @@ -802,30 +872,8 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, lex_string_set(trg_db_cl_name, get_default_db_collation(thd, tables->db)->name);
- /* - Create well-formed trigger definition query. Original query is not - appropriated, because definer-clause can be not truncated. - */ - - stmt_query->append(STRING_WITH_LEN("CREATE ")); - - if (lex->sphead->m_chistics->suid != SP_IS_NOT_SUID) - { - /* - Append definer-clause if the trigger is SUID (a usual trigger in - new MySQL versions). - */ - - append_definer(thd, stmt_query, &definer_user, &definer_host); - } - - 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); - - stmt_query->append(stmt_definition.str, stmt_definition.length); + build_trig_stmt_query(thd, tables, stmt_query, + trg_definer, trg_definer_holder);
trg_definer and trg_definer_holder don't seem to be used anywhere
trg_def->str= stmt_query->c_ptr_safe(); trg_def->length= stmt_query->length();
Regards, Sergei
Hi Sergei, On 02/26/2015 08:02 PM, Sergei Golubchik wrote:
Hi, Alexander!
Please review a patch for MDEV-7286.
Pretty good. I have just a couple of comments about build_trig_stmt_query() usage, see below.
Thanks. A fixed version is attached. See my comments inline:
diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc index 4b20813..d3713cc 100644 --- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -608,6 +608,67 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
<cut>
+ + /* + Create well-formed trigger definition query. Original query is not + appropriated, because definer-clause can be not truncated.
you don't have to preserve old comments verbatim, feel free to fix them as you see fit. E.g. here I'd say "is not appropriate" or, better, rewrite the whole comment to make sense (what does it mean "definer-clause can be not truncated"?)
Changed the comment to: /* Create a query with the full trigger definition. The original query is not appropriate, as it can miss the DEFINER=XXX part. */
+ */ + stmt_query->append(STRING_WITH_LEN("CREATE ")); + + if (lex->create_info.or_replace()) + stmt_query->append(STRING_WITH_LEN("OR REPLACE ")); + + if (lex->sphead->m_chistics->suid != SP_IS_NOT_SUID) + { + /* SUID trigger */ + lex->definer->set_lex_string(trg_definer, trg_definer_holder); + /* + Append definer-clause if the trigger is SUID (a usual trigger in + new MySQL versions).
And this comment I'd remove completely. "new MySQL versions" isn't true anymore. And what's left is "Append definer" one line before a function append_definer() is called. Useless.
Removed. <cut>
@@ -722,8 +794,29 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, /* Use the filesystem to enforce trigger namespace constraints. */ if (!access(trigname_buff, F_OK)) { - my_error(ER_TRG_ALREADY_EXISTS, MYF(0)); - return 1; + if (lex->create_info.or_replace()) + { + String drop_trg_query; + drop_trg_query.append("DROP TRIGGER "); + drop_trg_query.append(lex->spname->m_name.str); + if (drop_trigger(thd, tables, &drop_trg_query)) + return 1; + } + else if (lex->create_info.if_not_exists()) + { + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, + ER_TRG_ALREADY_EXISTS, ER(ER_TRG_ALREADY_EXISTS), + trigname_buff); + LEX_STRING trg_definer_tmp; + build_trig_stmt_query(thd, tables, stmt_query, + &trg_definer_tmp, trg_definer_holder);
Why?
The caller expects stmt_query to be properly filled on "false" returned. Note, although the trigger exists on the master, we still put the "CREATE IF NOT EXISTS" query into the binary log, to create it on the slave in case if it's missing.
+ return false; + } + else + { + my_error(ER_TRG_ALREADY_EXISTS, MYF(0)); + return true; + } }
trigname.trigger_table.str= tables->table_name; @@ -802,30 +872,8 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables, lex_string_set(trg_db_cl_name, get_default_db_collation(thd, tables->db)->name);
- /* - Create well-formed trigger definition query. Original query is not - appropriated, because definer-clause can be not truncated. - */ - - stmt_query->append(STRING_WITH_LEN("CREATE ")); - - if (lex->sphead->m_chistics->suid != SP_IS_NOT_SUID) - { - /* - Append definer-clause if the trigger is SUID (a usual trigger in - new MySQL versions). - */ - - append_definer(thd, stmt_query, &definer_user, &definer_host); - } - - 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); - - stmt_query->append(stmt_definition.str, stmt_definition.length); + build_trig_stmt_query(thd, tables, stmt_query, + trg_definer, trg_definer_holder);
trg_definer and trg_definer_holder don't seem to be used anywhere
Sorry, I think they are still used. trg_definer is used here: !(trg_definer= alloc_lex_string(&table->mem_root)) || definers_list.push_back(trg_definer, &table->mem_root) || and it points to thr_definer_holder (in case of a SUID trigger) or is equal to emptry_lex_string (in case of a non-SUID trigger). By the way, this hack with trg_definer and trg_definer_holder also looked hard to read for me. I'd better wrap them into a single structure: struct Definer_with_buffer { LEX_STRING *definer; char definer_holder[USER_HOST_BUFF_SIZE]; }; and change build_trig_stmt_query() interface to: static void build_trig_stmt_query(THD *thd, TABLE_LIST *tables, String *stmt_query, Definer_with_buffer *trg_definer); Note, this Definer_with_buffer could be used in more places, e.g. in a similar place in sp.cc.
trg_def->str= stmt_query->c_ptr_safe(); trg_def->length= stmt_query->length();
Regards, Sergei
Hi, Alexander! On Feb 27, Alexander Barkov wrote:
+ /* + Create well-formed trigger definition query. Original query is not + appropriated, because definer-clause can be not truncated.
you don't have to preserve old comments verbatim, feel free to fix them as you see fit. E.g. here I'd say "is not appropriate" or, better, rewrite the whole comment to make sense (what does it mean "definer-clause can be not truncated"?)
Changed the comment to:
/* Create a query with the full trigger definition. The original query is not appropriate, as it can miss the DEFINER=XXX part. */
Ah! Now I understand the original comment, "can be not truncated" apparently means "cannot be omitted, as it might've been in the original query" :) You variant is clear enough, thanks. Other questions - you're right. I didn't review your new patch, as I think you haven't change anything besides comments. If that was wrong and you want me to review it - just say so. Otherwise ok to push, thanks. Regards, Sergei
participants (4)
-
Alexander Barkov -
Geoff Montee -
Peter Laursen -
Sergei Golubchik