maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Monty!

On May 13, Michael Widenius wrote:
> revision-id: 094ae6bbcc0 (mariadb-10.6.0-79-g094ae6bbcc0)
> parent(s): c80f867b0df
> author: Michael Widenius <michael.widenius@xxxxxxxxx>
> committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> timestamp: 2021-05-04 21:09:11 +0300
> message:
> 
> MDEV-24395 Atomic DROP TRIGGER

> diff --git a/sql/ddl_log.cc b/sql/ddl_log.cc
> index 6145b2f7318..cf65f8ad876 100644
> --- a/sql/ddl_log.cc
> +++ b/sql/ddl_log.cc
> @@ -1265,6 +1290,70 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
>      }
>      break;
>    }
> +  case DDL_LOG_DROP_TRIGGER_ACTION:
> +  {
> +    MY_STAT stat_info;
> +    off_t frm_length= 1;                        // Impossible length
> +    LEX_CSTRING thd_db= thd->db;
> +
> +    /* Delete trigger temporary file if it still exists */
> +    if (!build_filename_and_delete_tmp_file(to_path, sizeof(to_path) - 1,
> +                                            &ddl_log_entry->db,
> +                                            &ddl_log_entry->name,
> +                                            TRG_EXT,
> +                                            key_file_fileparser))
> +    {
> +      /* Temporary file existed and was deleted, nothing left to do */

you didn't answer my question in one of the previous reviews,
what is a temporary file *~ ? What creates it and where?

> +      (void) update_phase(entry_pos, DDL_LOG_FINAL_PHASE);
> +      break;
> +    }
> +    /*
> +      We can use length of TRG file as an indication if trigger was removed.
> +      If there is no file, then it means that this was the last trigger
> +      and the file was removed.
> +     */
> +    if (my_stat(to_path, &stat_info, MYF(0)))
> +      frm_length= (off_t) stat_info.st_size;
> +    if (frm_length != (off_t) ddl_log_entry->unique_id &&
> +        mysql_bin_log.is_open())
> +    {
> +      /*
> +        File size changed and it was not binlogged (as this entry was
> +        executed)
> +      */
> +      (void) rm_trigname_file(to_path, &ddl_log_entry->db,
> +                              &ddl_log_entry->from_name,
> +                              MYF(0));
> +
> +      ddl_drop_query.length(0);
> +      ddl_drop_query.set_charset(system_charset_info);
> +      if (ddl_log_entry->tmp_name.length)

I'm not sure it's a good idea. It'll have some rather confusing
conditional behavior when sometimes a query is logged
with the original user's comment. And adding a couple of#
characters to the comment makes the comment to disappear because the
query will be replaced with a generated one.

It might've been more predictable and less confusing, if the query
would be always generated. After all, it's only for the ddl recovery case,
it's reasonable to see a generated query for redo actions that happen
during the crash recovery.

> +      {
> +        /* We can use the original query */
> +        ddl_drop_query.append(&ddl_log_entry->tmp_name);
> +      }
> +      else
> +      {
> +        /* Generate new query */
> +        ddl_drop_query.append(STRING_WITH_LEN("DROP TRIGGER IF EXISTS "));
> +        append_identifier(thd, &ddl_drop_query, &ddl_log_entry->from_name);
> +        ddl_drop_query.append(&end_comment);
> +      }
> +      if (mysql_bin_log.is_open())
> +      {
> +        mysql_mutex_unlock(&LOCK_gdl);
> +        thd->db= ddl_log_entry->db;
> +        (void) thd->binlog_query(THD::STMT_QUERY_TYPE,
> +                                 ddl_drop_query.ptr(),
> +                                 ddl_drop_query.length(), TRUE, FALSE,
> +                                 FALSE, 0);
> +        thd->db= thd_db;
> +        mysql_mutex_lock(&LOCK_gdl);
> +      }
> +    }
> +    (void) update_phase(entry_pos, DDL_LOG_FINAL_PHASE);
> +    break;
> +  }
>    default:
>      DBUG_ASSERT(0);
>      break;

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx