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@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> 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@mariadb.org