Re: [Maria-developers] c80f867b0df: MDEV-23844 Atomic DROP TABLE
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Michael! On May 12, Michael Widenius wrote:
revision-id: c80f867b0df (mariadb-10.6.0-78-gc80f867b0df) parent(s): eebebe8ef75 author: Michael Widenius
committer: Michael Widenius timestamp: 2021-05-04 21:09:11 +0300 message: MDEV-23844 Atomic DROP TABLE ... diff --git a/sql/sql_view.cc b/sql/sql_view.cc index e6d726b30d7..ff8aa55adef 100644 --- a/sql/sql_view.cc +++ b/sql/sql_view.cc @@ -1861,8 +1868,18 @@ bool mysql_drop_view(THD *thd, TABLE_LIST *views, enum_drop_mode drop_mode) not_exists_count++; continue; } + if (!first_table++)
yuck. so your "first_table" actually means "not_first_view", because when it's FALSE, it's the first view. why not to rename it to "not_first_view" ?
+ { + if (ddl_log_drop_view_init(thd, &ddl_log_state)) + DBUG_RETURN(TRUE);
what about a crash here? the first entry is written, the second is not.
+ } + if (ddl_log_drop_view(thd, &ddl_log_state, &cpath, &view->db, + &view->table_name)) + DBUG_RETURN(TRUE); + debug_crash_here("ddl_log_drop_before_delete_view"); if (unlikely(mysql_file_delete(key_file_frm, path, MYF(MY_WME)))) delete_error= TRUE; + debug_crash_here("ddl_log_drop_after_delete_view");
some_views_deleted= TRUE;
diff --git a/sql/log.cc b/sql/log.cc index 6e0a9706ec8..dba999054e4 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -10471,16 +10483,16 @@ int TC_LOG_BINLOG::recover(LOG_INFO *linfo, const char *last_log_name, bool last_gtid_standalone= false; bool last_gtid_valid= false; #endif + DBUG_ENTER("TC_LOG_BINLOG::recover");
if (! fdle->is_valid() || - (do_xa && my_hash_init(key_memory_binlog_recover_exec, &xids, - &my_charset_bin, TC_LOG_PAGE_SIZE/3, 0, - sizeof(my_xid), 0, 0, MYF(0)))) + (my_hash_init(key_memory_binlog_recover_exec, &xids, + &my_charset_bin, TC_LOG_PAGE_SIZE/3, 0, + sizeof(my_xid), 0, 0, MYF(0)))) goto err1;
- if (do_xa) - init_alloc_root(key_memory_binlog_recover_exec, &mem_root, - TC_LOG_PAGE_SIZE, TC_LOG_PAGE_SIZE, MYF(0)); + init_alloc_root(key_memory_binlog_recover_exec, &mem_root, + TC_LOG_PAGE_SIZE, TC_LOG_PAGE_SIZE, MYF(0));
I don't think these two changes are correct. do_xa is TRUE on crash recovery and FALSE on gtid state recovery on 5.5->10.0 upgade or if gtid state file was deleted. There is no point to initialize the xid hash or mem_root, as it's not a crash recovery and there can be no incomplete transactions or ddl statements. Yes, I've seen the commit comment:
- TC_LOG_BINLOG::recover() was changed to always collect Xid's from the binary log and always call ddl_log_close_binlogged_events(). This was needed to be able to collect DROP TABLE events with embedded Xid's (used by ddl log).
but I cannot understand how it applies here.
fdle->flags&= ~LOG_EVENT_BINLOG_IN_USE_F; // abort on the first error
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 7a88ffdfd1b..0a83396ae14 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -1363,6 +1373,17 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists,
thd->replication_flags= 0; was_view= table_type == TABLE_TYPE_VIEW; + + if (!first_table++)
first_table -> not_first_table
+ { + LEX_CSTRING comment= {comment_start, (size_t) comment_len}; + if (ddl_log_drop_table_init(thd, &ddl_log_state, &comment)) + { + error= 1; + goto err; + } + } + if ((table_type == TABLE_TYPE_UNKNOWN) || (was_view && !drop_view) || (table_type != TABLE_TYPE_SEQUENCE && drop_sequence)) { @@ -1565,6 +1611,7 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, table_name.str, (uint)table_name.length); mysql_audit_drop_table(thd, table); } + ddl_log_update_phase(&ddl_log_state, DDL_DROP_PHASE_COLLECT);
better call it DDL_DROP_PHASE_BINLOG. It would've saved me ~10 minutes reading the code back and forth, as ""DDL_DROP_PHASE_BINLOG" immediately explains what this phase is for.
if (!dont_log_query && (!error || table_dropped || non_existing_table_error(error))) diff --git a/sql/ddl_log.h b/sql/ddl_log.h index 316e6708f22..b843404fc3c 100644 --- a/sql/ddl_log.h +++ b/sql/ddl_log.h @@ -97,6 +101,14 @@ enum enum_ddl_log_rename_table_phase { DDL_RENAME_PHASE_TABLE, };
+enum enum_ddl_log_drop_table_phase { + DDL_DROP_PHASE_TABLE=0, + DDL_DROP_PHASE_TRIGGER,
no phase for deleting data from stat tables?
+ DDL_DROP_PHASE_COLLECT, + DDL_DROP_PHASE_RESET, /* Reset found list of dropped tables */
this one is unused (also unused in further patches, I've checked)
+ DDL_DROP_PHASE_END +}; + /* Setting ddl_log_entry.phase to this has the same effect as setting action_type to DDL_IGNORE_LOG_ENTRY_CODE diff --git a/sql/ddl_log.cc b/sql/ddl_log.cc index 777dfdddbc7..6145b2f7318 100644 --- a/sql/ddl_log.cc +++ b/sql/ddl_log.cc @@ -109,6 +112,7 @@ struct st_global_ddl_log };
st_global_ddl_log global_ddl_log; +String ddl_drop_query; // Used during startup recovery
static
mysql_mutex_t LOCK_gdl;
@@ -1132,6 +1149,122 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root, (void) update_phase(entry_pos, DDL_LOG_FINAL_PHASE); } break; + case DDL_LOG_DROP_TABLE_INIT_ACTION: + { + LEX_CSTRING *comment= &ddl_log_entry->tmp_name; + ddl_drop_query.length(0); + ddl_drop_query.set_charset(system_charset_info); + ddl_drop_query.append(STRING_WITH_LEN("DROP TABLE IF EXISTS ")); + if (comment->length) + { + ddl_drop_query.append(comment); + ddl_drop_query.append(' '); + } + /* We don't increment phase as we want to retry this in case of crash */ + break; + } + case DDL_LOG_DROP_TABLE_ACTION: + { + LEX_CSTRING db, table, path; + db= ddl_log_entry->db; + table= ddl_log_entry->name; + /* Note that path is without .frm extension */ + path= ddl_log_entry->tmp_name; + + switch (ddl_log_entry->phase) { + case DDL_DROP_PHASE_TABLE: + if (hton) + { + if ((error= hton->drop_table(hton, path.str))) + { + if (!non_existing_table_error(error)) + break; + error= -1; + } + } + else + error= ha_delete_table_force(thd, path.str, &db, &table); + if (error <= 0) + { + /* Not found or already deleted. Delete .frm if it exists */ + strxnmov(to_path, sizeof(to_path)-1, path.str, reg_ext, NullS); + mysql_file_delete(key_file_frm, to_path, MYF(MY_WME|MY_IGNORE_ENOENT));
this should rather be inside the previous if(). no need to do it for ha_delete_table_force(), as it doesn't leave any traces and removes all remnants of a table.
+ } + if (ddl_log_increment_phase_no_lock(entry_pos)) + break; + (void) ddl_log_sync_no_lock();
No need to, ddl_log_increment_phase_no_lock() syncs internally
+ /* Fall through */ + case DDL_DROP_PHASE_TRIGGER: + Table_triggers_list::drop_all_triggers(thd, &db, &table, + MYF(MY_WME | MY_IGNORE_ENOENT)); + if (ddl_log_increment_phase_no_lock(entry_pos)) + break; + (void) ddl_log_sync_no_lock();
same. and in other places too, I'd expect.
+ /* Fall through */ + + case DDL_DROP_PHASE_COLLECT:
indentation?
+ append_identifier(thd, &ddl_drop_query, &db); + ddl_drop_query.append('.'); + append_identifier(thd, &ddl_drop_query, &table); + ddl_drop_query.append(','); + /* We don't increment phase as we want to retry this in case of crash */ + + if (!ddl_log_entry->next_entry && mysql_bin_log.is_open()) + { + /* Last drop table. Write query to binlog */ + LEX_CSTRING end_comment= + { STRING_WITH_LEN(" /* generated by ddl log */")};
"generated by ddl log" ? a "log" cannot "generate" a statement, may be "generated during ddl recovery" ?
+ ddl_drop_query.length(ddl_drop_query.length()-1); + ddl_drop_query.append(&end_comment); + + mysql_mutex_unlock(&LOCK_gdl); + (void) thd->binlog_query(THD::STMT_QUERY_TYPE, ddl_drop_query.ptr(), + ddl_drop_query.length(), TRUE, FALSE, + FALSE, 0); + mysql_mutex_lock(&LOCK_gdl); + } + break; + } + break; + } + case DDL_LOG_DROP_VIEW_INIT_ACTION: + { + ddl_drop_query.length(0); + ddl_drop_query.set_charset(system_charset_info); + ddl_drop_query.append(STRING_WITH_LEN("DROP VIEW IF EXISTS ")); + /* We don't increment phase as we want to retry this in case of crash */ + break; + } + case DDL_LOG_DROP_VIEW_ACTION: + { + LEX_CSTRING db, table, path; + db= ddl_log_entry->db; + table= ddl_log_entry->name; + /* Note that for views path is WITH .frm extension */ + path= ddl_log_entry->tmp_name; + + mysql_file_delete(key_file_frm, path.str, MYF(MY_WME|MY_IGNORE_ENOENT)); + append_identifier(thd, &ddl_drop_query, &db); + ddl_drop_query.append('.'); + append_identifier(thd, &ddl_drop_query, &table); + ddl_drop_query.append(','); + + if (!ddl_log_entry->next_entry) + { + /* Last drop view. Write query to binlog */ + LEX_CSTRING end_comment= + { STRING_WITH_LEN(" /* generated by ddl log */")}; + ddl_drop_query.length(ddl_drop_query.length()-1); + ddl_drop_query.append(&end_comment); + + mysql_mutex_unlock(&LOCK_gdl); + (void) thd->binlog_query(THD::STMT_QUERY_TYPE, ddl_drop_query.ptr(), + ddl_drop_query.length(), TRUE, FALSE, + FALSE, 0); + mysql_mutex_lock(&LOCK_gdl); + } + break; + } default: DBUG_ASSERT(0); break; @@ -1682,6 +1823,7 @@ void ddl_log_release_entries(DDL_LOG_STATE *ddl_log_state) next= log_entry->next_active_log_entry; ddl_log_release_memory_entry(log_entry); } + ddl_log_state->list= 0;
may be you should also clear ddl_log_state->execute_entry below?
if (ddl_log_state->execute_entry) ddl_log_release_memory_entry(ddl_log_state->execute_entry); @@ -1776,6 +1918,33 @@ bool ddl_log_update_xid(DDL_LOG_STATE *state, ulonglong xid) }
+/* + Write ddl_log_entry and write or update ddl_execute_entry +*/ + +static bool ddl_log_write(DDL_LOG_STATE *ddl_state, + DDL_LOG_ENTRY *ddl_log_entry)
could you use a more descriptive name, please?
+{ + int error; + DDL_LOG_MEMORY_ENTRY *log_entry; + DBUG_ENTER("ddl_log_write"); + + mysql_mutex_lock(&LOCK_gdl); + error= ((ddl_log_write_entry(ddl_log_entry, &log_entry)) || + ddl_log_write_execute_entry(log_entry->entry_pos, + &ddl_state->execute_entry)); + mysql_mutex_unlock(&LOCK_gdl); + if (error) + { + if (log_entry) + ddl_log_release_memory_entry(log_entry); + DBUG_RETURN(1); + } + add_log_entry(ddl_state, log_entry); + DBUG_RETURN(0); +} + + /** Logging of rename table */
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/68cce0b6e372c2c66f4d66c6356102af.jpg?s=120&d=mm&r=g)
Hi!
On Wed, May 12, 2021 at 3:18 PM Sergei Golubchik
Hi, Michael!
On May 12, Michael Widenius wrote:
revision-id: c80f867b0df (mariadb-10.6.0-78-gc80f867b0df) parent(s): eebebe8ef75 author: Michael Widenius
committer: Michael Widenius timestamp: 2021-05-04 21:09:11 +0300 message: MDEV-23844 Atomic DROP TABLE ... diff --git a/sql/sql_view.cc b/sql/sql_view.cc index e6d726b30d7..ff8aa55adef 100644 --- a/sql/sql_view.cc +++ b/sql/sql_view.cc @@ -1861,8 +1868,18 @@ bool mysql_drop_view(THD *thd, TABLE_LIST *views, enum_drop_mode drop_mode) not_exists_count++; continue; } + if (!first_table++)
yuck. so your "first_table" actually means "not_first_view", because when it's FALSE, it's the first view.
why not to rename it to "not_first_view" ?
I have renamed it to view_count. I prefer to use a counter as it is easer to set and test at the same time. Also, when debugging it also helps to see where one is.
+ { + if (ddl_log_drop_view_init(thd, &ddl_log_state)) + DBUG_RETURN(TRUE);
what about a crash here? the first entry is written, the second is not.
The init creates the bases for a set of views to drop. We do init once and then we only need to do ddl_log_drop_view for the rest. Most ddl_log operations has only one call. Only those that can have multiple operations, like drop or rename has an init call.
+ } + if (ddl_log_drop_view(thd, &ddl_log_state, &cpath, &view->db, + &view->table_name)) + DBUG_RETURN(TRUE);
<cut>
diff --git a/sql/log.cc b/sql/log.cc index 6e0a9706ec8..dba999054e4 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -10471,16 +10483,16 @@ int TC_LOG_BINLOG::recover(LOG_INFO *linfo, const char *last_log_name, bool last_gtid_standalone= false; bool last_gtid_valid= false; #endif + DBUG_ENTER("TC_LOG_BINLOG::recover");
if (! fdle->is_valid() || - (do_xa && my_hash_init(key_memory_binlog_recover_exec, &xids, - &my_charset_bin, TC_LOG_PAGE_SIZE/3, 0, - sizeof(my_xid), 0, 0, MYF(0)))) + (my_hash_init(key_memory_binlog_recover_exec, &xids, + &my_charset_bin, TC_LOG_PAGE_SIZE/3, 0, + sizeof(my_xid), 0, 0, MYF(0)))) goto err1;
- if (do_xa) - init_alloc_root(key_memory_binlog_recover_exec, &mem_root, - TC_LOG_PAGE_SIZE, TC_LOG_PAGE_SIZE, MYF(0)); + init_alloc_root(key_memory_binlog_recover_exec, &mem_root, + TC_LOG_PAGE_SIZE, TC_LOG_PAGE_SIZE, MYF(0));
I don't think these two changes are correct. do_xa is TRUE on crash recovery and FALSE on gtid state recovery on 5.5->10.0 upgade or if gtid state file was deleted.
The problem is that do_xa is not always true on crash recovery.
For example if the following is not true in do_binlog_recovery:
if (ev->flags & LOG_EVENT_BINLOG_IN_USE_F)
It will execute binglog recovery later with do_xa_recovery == false trough
the following code inTC_LOG_BINLOG::open(const char *opt_name)
if (!binlog_state_recover_done)
{
binlog_state_recover_done= true;
if (do_binlog_recovery(opt_bin_logname, false))
DBUG_RETURN(1);
}
Here is a trace:
#0 MYSQL_BIN_LOG::recover (this=0x2783d80
There is no point to initialize the xid hash or mem_root, as it's not a crash recovery and there can be no incomplete transactions or ddl statements.
Yes, I've seen the commit comment:
- TC_LOG_BINLOG::recover() was changed to always collect Xid's from the binary log and always call ddl_log_close_binlogged_events(). This was needed to be able to collect DROP TABLE events with embedded Xid's (used by ddl log).
but I cannot understand how it applies here.
The issue is that because of ddl log, ::recover must ALWAYS collect binlog 'xid's to be able to do ddl log recovery at all. You can verify this by reversing the patch and do: mtr atomic.create_view This will fail with a different result that shows that ddl recovery was reverting things wrongly because it could not find the xid's in the binlog (as they where not collected)
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 7a88ffdfd1b..0a83396ae14 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -1363,6 +1373,17 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists,
thd->replication_flags= 0; was_view= table_type == TABLE_TYPE_VIEW; + + if (!first_table++)
first_table -> not_first_table
changed to table_count
+ { + LEX_CSTRING comment= {comment_start, (size_t) comment_len}; + if (ddl_log_drop_table_init(thd, &ddl_log_state, &comment)) + { + error= 1; + goto err; + } + } + if ((table_type == TABLE_TYPE_UNKNOWN) || (was_view && !drop_view) || (table_type != TABLE_TYPE_SEQUENCE && drop_sequence)) { @@ -1565,6 +1611,7 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, table_name.str, (uint)table_name.length); mysql_audit_drop_table(thd, table); } + ddl_log_update_phase(&ddl_log_state, DDL_DROP_PHASE_COLLECT);
better call it DDL_DROP_PHASE_BINLOG. It would've saved me ~10 minutes reading the code back and forth, as ""DDL_DROP_PHASE_BINLOG" immediately explains what this phase is for.
Will do. Not sure where PHASE_COLLECT come from...
if (!dont_log_query && (!error || table_dropped || non_existing_table_error(error))) diff --git a/sql/ddl_log.h b/sql/ddl_log.h index 316e6708f22..b843404fc3c 100644 --- a/sql/ddl_log.h +++ b/sql/ddl_log.h @@ -97,6 +101,14 @@ enum enum_ddl_log_rename_table_phase { DDL_RENAME_PHASE_TABLE, };
+enum enum_ddl_log_drop_table_phase { + DDL_DROP_PHASE_TABLE=0, + DDL_DROP_PHASE_TRIGGER,
no phase for deleting data from stat tables?
+ DDL_DROP_PHASE_COLLECT, + DDL_DROP_PHASE_RESET, /* Reset found list of dropped tables */
this one is unused (also unused in further patches, I've checked)
No it isn't. case DDL_DROP_PHASE_COLLECT: .... if (increment_phase(entry_pos)) case DDL_DROP_PHASE_RESET: The increment_phase changes current phase on disk to the next one that is PHASE_RESET and it will be used if there is a crash in the next phase.
+ DDL_DROP_PHASE_END +}; + /* Setting ddl_log_entry.phase to this has the same effect as setting action_type to DDL_IGNORE_LOG_ENTRY_CODE diff --git a/sql/ddl_log.cc b/sql/ddl_log.cc index 777dfdddbc7..6145b2f7318 100644 --- a/sql/ddl_log.cc +++ b/sql/ddl_log.cc @@ -109,6 +112,7 @@ struct st_global_ddl_log };
st_global_ddl_log global_ddl_log; +String ddl_drop_query; // Used during startup recovery
static
Fixed in later commits where all ddl log variables are moved to recovery_state <cut>
+ case DDL_LOG_DROP_TABLE_ACTION: + { + LEX_CSTRING db, table, path; + db= ddl_log_entry->db; + table= ddl_log_entry->name; + /* Note that path is without .frm extension */ + path= ddl_log_entry->tmp_name; + + switch (ddl_log_entry->phase) { + case DDL_DROP_PHASE_TABLE: + if (hton) + { + if ((error= hton->drop_table(hton, path.str))) + { + if (!non_existing_table_error(error)) + break; + error= -1; + } + } + else + error= ha_delete_table_force(thd, path.str, &db, &table); + if (error <= 0) + { + /* Not found or already deleted. Delete .frm if it exists */ + strxnmov(to_path, sizeof(to_path)-1, path.str, reg_ext, NullS); + mysql_file_delete(key_file_frm, to_path, MYF(MY_WME|MY_IGNORE_ENOENT));
this should rather be inside the previous if(). no need to do it for ha_delete_table_force(), as it doesn't leave any traces and removes all remnants of a table.
ha_delete_table_force does not delete the .frm file.
+ } + if (ddl_log_increment_phase_no_lock(entry_pos)) + break; + (void) ddl_log_sync_no_lock();
No need to, ddl_log_increment_phase_no_lock() syncs internally
Fixed in later commits.
+ /* Fall through */ + case DDL_DROP_PHASE_TRIGGER: + Table_triggers_list::drop_all_triggers(thd, &db, &table, + MYF(MY_WME | MY_IGNORE_ENOENT)); + if (ddl_log_increment_phase_no_lock(entry_pos)) + break; + (void) ddl_log_sync_no_lock();
same. and in other places too, I'd expect.
All fixed in later commits
+ /* Fall through */ + + case DDL_DROP_PHASE_COLLECT:
indentation?
Fixed by later commits.
+ append_identifier(thd, &ddl_drop_query, &db); + ddl_drop_query.append('.'); + append_identifier(thd, &ddl_drop_query, &table); + ddl_drop_query.append(','); + /* We don't increment phase as we want to retry this in case of crash */ + + if (!ddl_log_entry->next_entry && mysql_bin_log.is_open()) + { + /* Last drop table. Write query to binlog */ + LEX_CSTRING end_comment= + { STRING_WITH_LEN(" /* generated by ddl log */")};
"generated by ddl log" ? a "log" cannot "generate" a statement, may be "generated during ddl recovery" ?
I was thinking about ddl_log.cc here. But agree that ddl recovery is more clear. Fixed. <cut>
default: DBUG_ASSERT(0); break; @@ -1682,6 +1823,7 @@ void ddl_log_release_entries(DDL_LOG_STATE *ddl_log_state) next= log_entry->next_active_log_entry; ddl_log_release_memory_entry(log_entry); } + ddl_log_state->list= 0;
may be you should also clear ddl_log_state->execute_entry below?
Has never been a problem in the code as everything has been procteced by ddl_log_state->list. But agree it is a good idea, just in case. Will add it.
if (ddl_log_state->execute_entry) ddl_log_release_memory_entry(ddl_log_state->execute_entry); @@ -1776,6 +1918,33 @@ bool ddl_log_update_xid(DDL_LOG_STATE *state, ulonglong xid) }
+/* + Write ddl_log_entry and write or update ddl_execute_entry +*/ + +static bool ddl_log_write(DDL_LOG_STATE *ddl_state, + DDL_LOG_ENTRY *ddl_log_entry)
could you use a more descriptive name, please?
Not for this one. I would have to go and change every atomic commit for this to work and I don't have time for it now. Can be done later as it has to be a separate commit anyway because of the above reason. I also didn't come up with a very good name for it :( ddl_log_finalize ? ddl_log_write_entry_and_exceute_entry? Neither much better... Regards, Monty
participants (2)
-
Michael Widenius
-
Sergei Golubchik