maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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