Skip to content

Commit

Permalink
Check if we can rename triggers before doing an ALTER TABLE ... RENAME
Browse files Browse the repository at this point in the history 
ALTER TABLE .. RENAME, when used with the inplace algorithm, does:
- Do an inplace or online alter to the new definition
- Rename to new name
- Update triggers.

If update triggers would fail, we would rename the table back.
The problem with this approach is that the table would have the new
definition but the rename would fail.  The binary log would also not be
updated.

The solution to this is to very early check if we can rename triggers
and give an error if this would fail.
Both ALTER TABLE ... RENAME and RENAME TABLE is fixed.

This was implemented by moving the pre-check of rename table in triggers
from Table_triggers_list::change_table_name() to
Table_triggers_list::prepare_for_rename().
  • Loading branch information
@montywi @vuvova
montywi authored and vuvova committed May 19, 2021
1 parent c844a76 commit 3c578b0
Show file tree
Hide file tree
Showing 5 changed files with 176 additions and 63 deletions.
12 changes: 9 additions & 3 deletions sql/ddl_log.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1160,6 +1160,7 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
case DDL_RENAME_PHASE_TRIGGER:
{
MDL_request mdl_request;
TRIGGER_RENAME_PARAM trigger_param;

build_filename_and_delete_tmp_file(to_path, sizeof(to_path),
&ddl_log_entry->db,
Expand Down Expand Up @@ -1195,14 +1196,20 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
error= thd->mdl_context.acquire_lock(&mdl_request, 1);
/* acquire_locks() should never fail during recovery */
DBUG_ASSERT(error == 0);

(void) Table_triggers_list::prepare_for_rename(thd,
&trigger_param,
&ddl_log_entry->db,
&to_table,
&to_converted_name,
&ddl_log_entry->from_db,
&from_table);
(void) Table_triggers_list::change_table_name(thd,
&trigger_param,
&ddl_log_entry->db,
&to_table,
&to_converted_name,
&ddl_log_entry->from_db,
&from_table);

thd->mdl_context.release_lock(mdl_request.ticket);
}
if (ddl_log_increment_phase_no_lock(entry_pos))
Expand Down Expand Up @@ -1726,7 +1733,6 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
}
break;
}
}
default:
DBUG_ASSERT(0);
break;
Expand Down
16 changes: 14 additions & 2 deletions sql/sql_rename.cc
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
Copyright (c) 2000, 2013, Oracle and/or its affiliates.
Copyright (c) 2011, 2013, Monty Program Ab.
Copyright (c) 2011, 2021, Monty Program Ab.
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -339,6 +339,7 @@ do_rename(THD *thd, rename_param *param, DDL_LOG_STATE *ddl_log_state,
int rc= 1;
handlerton *hton;
LEX_CSTRING *old_alias, *new_alias;
TRIGGER_RENAME_PARAM rename_param;
DBUG_ENTER("do_rename");
DBUG_PRINT("enter", ("skip_error: %d", (int) skip_error));

Expand All @@ -361,6 +362,15 @@ do_rename(THD *thd, rename_param *param, DDL_LOG_STATE *ddl_log_state,
if (hton->flags & HTON_TABLE_MAY_NOT_EXIST_ON_SLAVE)
*force_if_exists= 1;

/* Check if we can rename triggers */
if (Table_triggers_list::prepare_for_rename(thd, &rename_param,
&ren_table->db,
old_alias,
&ren_table->table_name,
new_db,
new_alias))
DBUG_RETURN(!skip_error);

thd->replication_flags= 0;

if (ddl_log_rename_table(thd, ddl_log_state, hton,
Expand All @@ -379,7 +389,9 @@ do_rename(THD *thd, rename_param *param, DDL_LOG_STATE *ddl_log_state,

debug_crash_here("ddl_log_rename_before_rename_trigger");

if (!(rc= Table_triggers_list::change_table_name(thd, &ren_table->db,
if (!(rc= Table_triggers_list::change_table_name(thd,
&rename_param,
&ren_table->db,
old_alias,
&ren_table->table_name,
new_db,
Expand Down
32 changes: 25 additions & 7 deletions sql/sql_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7058,6 +7058,7 @@ static bool mysql_inplace_alter_table(THD *thd,
TABLE *altered_table,
Alter_inplace_info *ha_alter_info,
MDL_request *target_mdl_request,
TRIGGER_RENAME_PARAM *trigger_param,
Alter_table_ctx *alter_ctx)
{
Open_table_context ot_ctx(thd, MYSQL_OPEN_REOPEN | MYSQL_OPEN_IGNORE_KILLED);
Expand Down Expand Up @@ -7330,7 +7331,7 @@ static bool mysql_inplace_alter_table(THD *thd,
*/
DBUG_RETURN(true);
}
if (Table_triggers_list::change_table_name(thd,
if (Table_triggers_list::change_table_name(thd, trigger_param,
&alter_ctx->db,
&alter_ctx->alias,
&alter_ctx->table_name,
Expand All @@ -7343,7 +7344,8 @@ static bool mysql_inplace_alter_table(THD *thd,
*/
(void) mysql_rename_table(db_type,
&alter_ctx->new_db, &alter_ctx->new_alias,
&alter_ctx->db, &alter_ctx->alias, NO_FK_CHECKS);
&alter_ctx->db, &alter_ctx->alias,
NO_FK_CHECKS);
DBUG_RETURN(true);
}
rename_table_in_stat_tables(thd, &alter_ctx->db, &alter_ctx->alias,
Expand Down Expand Up @@ -8849,6 +8851,7 @@ simple_tmp_rename_or_index_change(THD *thd, TABLE_LIST *table_list,
static bool
simple_rename_or_index_change(THD *thd, TABLE_LIST *table_list,
Alter_info::enum_enable_or_disable keys_onoff,
TRIGGER_RENAME_PARAM *trigger_param,
Alter_table_ctx *alter_ctx)
{
TABLE *table= table_list->table;
Expand Down Expand Up @@ -8895,7 +8898,7 @@ simple_rename_or_index_change(THD *thd, TABLE_LIST *table_list,
if (mysql_rename_table(old_db_type, &alter_ctx->db, &alter_ctx->table_name,
&alter_ctx->new_db, &alter_ctx->new_alias, 0))
error= -1;
else if (Table_triggers_list::change_table_name(thd,
else if (Table_triggers_list::change_table_name(thd, trigger_param,
&alter_ctx->db,
&alter_ctx->alias,
&alter_ctx->table_name,
Expand Down Expand Up @@ -9080,6 +9083,7 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
MDL_request target_mdl_request;
MDL_ticket *mdl_ticket= 0;
Alter_table_prelocking_strategy alter_prelocking_strategy;
TRIGGER_RENAME_PARAM trigger_param;
DBUG_ENTER("mysql_alter_table");

/*
Expand Down Expand Up @@ -9504,6 +9508,16 @@ do_continue:;
create_info))
DBUG_RETURN(true);

/* Check if rename of triggers are supported */
if (alter_ctx.is_table_renamed() &&
Table_triggers_list::prepare_for_rename(thd, &trigger_param,
&alter_ctx.db,
&alter_ctx.alias,
&alter_ctx.table_name,
&alter_ctx.new_db,
&alter_ctx.new_alias))
DBUG_RETURN(true);

/*
Look if we have to do anything at all.
ALTER can become NOOP after handling
Expand Down Expand Up @@ -9549,6 +9563,7 @@ do_continue:;
}
res= simple_rename_or_index_change(thd, table_list,
alter_info->keys_onoff,
&trigger_param,
&alter_ctx);
}
else
Expand Down Expand Up @@ -9805,7 +9820,7 @@ do_continue:;
&altered_table))
goto err_new_table_cleanup;
/*
Avoid creating frm again in ha_create_table() if inline alter will not
Avoid creating frm again in ha_create_table() if inplace alter will not
be used.
*/
frm_is_created= 1;
Expand Down Expand Up @@ -9879,9 +9894,12 @@ do_continue:;
*/
enum_check_fields org_count_cuted_fields= thd->count_cuted_fields;
thd->count_cuted_fields= CHECK_FIELD_WARN;
int res= mysql_inplace_alter_table(thd, table_list, table, &altered_table,
int res= mysql_inplace_alter_table(thd,
table_list, table, &altered_table,
&ha_alter_info,
&target_mdl_request, &alter_ctx);
&target_mdl_request,
&trigger_param,
&alter_ctx);
thd->count_cuted_fields= org_count_cuted_fields;
my_free(const_cast<uchar*>(frm.str));

Expand Down Expand Up @@ -10216,7 +10234,7 @@ do_continue:;
// Check if we renamed the table and if so update trigger files.
if (alter_ctx.is_table_renamed())
{
if (Table_triggers_list::change_table_name(thd,
if (Table_triggers_list::change_table_name(thd, &trigger_param,
&alter_ctx.db,
&alter_ctx.alias,
&alter_ctx.table_name,
Expand Down
Loading

0 comments on commit 3c578b0

Please sign in to comment.