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