From 71964c76fe0d7266103025c31d5b7f5d50854383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Wed, 16 Jun 2021 09:03:02 +0300 Subject: MDEV-25910: Aim to make all InnoDB DDL durable Before any committed DDL operation returns control to the caller, we must ensure that it will have been durably committed before the ddl_log state may be changed, no matter if innodb_flush_log_at_trx_commit=0 is being used. Operations that would involve deleting files were already safe, because the durable write of the FILE_DELETE record that would precede the file deletion would also have made the commit durable. Furthermore, when we clean up ADD INDEX stubs that were left behind by a previous ha_innobase::commit_inplace_alter_table(commit=false) when MDL could not be acquired, we will use the same interface as for dropping indexes. This safety measure should be dead code, because ADD FULLTEXT INDEX is not supported online, and dropping indexes only involves file deletion for FULLTEXT INDEX. --- mysql-test/suite/atomic/alter_table.result | 2 +- storage/innobase/handler/ha_innodb.cc | 18 ++++++++-- storage/innobase/handler/handler0alter.cc | 58 +++++++++++++++++------------- 3 files changed, 49 insertions(+), 29 deletions(-) diff --git a/mysql-test/suite/atomic/alter_table.result b/mysql-test/suite/atomic/alter_table.result index 9807008e119..cd4c1ad5617 100644 --- a/mysql-test/suite/atomic/alter_table.result +++ b/mysql-test/suite/atomic/alter_table.result @@ -2134,7 +2134,7 @@ t1 CREATE TABLE `t1` ( KEY `a` (`a`) ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COMMENT='new' count(*) -0 +2 master-bin.000002 # Query # # use `test`; ALTER TABLE t1 ADD COLUMN c INT, ALGORITHM=copy, COMMENT "new" crash point: ddl_log_alter_after_rename_to_backup t1.frm diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 5dabab139b1..11e9ab2e8cd 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -1559,6 +1559,8 @@ retry: mem_heap_free(heap); for (pfs_os_file_t detached : to_close) os_file_close(detached); + /* Any changes must be persisted before we return. */ + log_write_up_to(mtr.commit_lsn(), true); } my_free(namebuf); @@ -13033,6 +13035,9 @@ ha_innobase::create( row_mysql_unlock_data_dictionary(trx); for (pfs_os_file_t d : deleted) os_file_close(d); error = info.create_table_update_dict(); + if (!(info.flags2() & DICT_TF2_TEMPORARY)) { + log_write_up_to(trx->commit_lsn, true); + } } if (own_trx) { @@ -13379,10 +13384,11 @@ err_exit: std::vector deleted; trx->commit(deleted); row_mysql_unlock_data_dictionary(trx); - if (trx != parent_trx) - trx->free(); for (pfs_os_file_t d : deleted) os_file_close(d); + log_write_up_to(trx->commit_lsn, true); + if (trx != parent_trx) + trx->free(); if (fts) purge_sys.resume_FTS(); DBUG_RETURN(0); @@ -13595,6 +13601,7 @@ int ha_innobase::truncate() err = create(name, table, &info, dict_table_is_file_per_table(ib_table), trx); + /* On success, create() durably committed trx. */ if (fts) { purge_sys.resume_FTS(); } @@ -13690,6 +13697,9 @@ ha_innobase::rename_table( } row_mysql_unlock_data_dictionary(trx); + if (error == DB_SUCCESS) { + log_write_up_to(trx->commit_lsn, true); + } trx->free(); if (error == DB_DUPLICATE_KEY) { @@ -15308,7 +15318,9 @@ ha_innobase::extra( break; case HA_EXTRA_END_ALTER_COPY: m_prebuilt->table->skip_alter_undo = 0; - log_write_up_to(LSN_MAX, true); + if (!m_prebuilt->table->is_temporary()) { + log_write_up_to(LSN_MAX, true); + } break; default:/* Do nothing */ ; diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index fce6c06b6c6..e020955502e 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -4090,6 +4090,26 @@ online_retry_drop_indexes_low( } } +/** After commit, unlock the data dictionary and close any deleted files. +@param deleted handles of deleted files +@param trx committed transaction */ +static void unlock_and_close_files(const std::vector &deleted, + trx_t *trx) +{ + row_mysql_unlock_data_dictionary(trx); + for (pfs_os_file_t d : deleted) + os_file_close(d); + log_write_up_to(trx->commit_lsn, true); +} + +/** Commit a DDL transaction and unlink any deleted files. */ +static void commit_unlock_and_unlink(trx_t *trx) +{ + std::vector deleted; + trx->commit(deleted); + unlock_and_close_files(deleted, trx); +} + /********************************************************************//** Drop any indexes that we were not able to free previously due to open table handles. */ @@ -4107,8 +4127,7 @@ online_retry_drop_indexes( row_mysql_lock_data_dictionary(trx); online_retry_drop_indexes_low(table, trx); - trx_commit_for_mysql(trx); - row_mysql_unlock_data_dictionary(trx); + commit_unlock_and_unlink(trx); trx->free(); } @@ -4139,7 +4158,16 @@ online_retry_drop_indexes_with_trx( trx_start_for_ddl(trx); online_retry_drop_indexes_low(table, trx); - trx_commit_for_mysql(trx); + std::vector deleted; + trx->commit(deleted); + /* FIXME: We are holding the data dictionary latch here + while waiting for the files to be actually deleted. + However, we should never have any deleted files here, + because they would be related to ADD FULLTEXT INDEX, + and that operation is never supported online. */ + for (pfs_os_file_t d : deleted) { + os_file_close(d); + } } } @@ -6094,24 +6122,6 @@ create_index_dict( DBUG_RETURN(index); } -/** After releasing the data dictionary latch, close deleted files -@param deleted handles of deleted files */ -static void close_unlinked_files(const std::vector &deleted) -{ - dict_sys.assert_not_locked(); - for (pfs_os_file_t d : deleted) - os_file_close(d); -} - -/** Commit a DDL transaction and unlink any deleted files. */ -static void commit_unlock_and_unlink(trx_t *trx) -{ - std::vector deleted; - trx->commit(deleted); - row_mysql_unlock_data_dictionary(trx); - close_unlinked_files(deleted); -} - /** Update internal structures with concurrent writes blocked, while preparing ALTER TABLE. @@ -11146,9 +11156,8 @@ foreign_fail: ctx->prebuilt->table, altered_table->s); } - row_mysql_unlock_data_dictionary(trx); + unlock_and_close_files(deleted, trx); trx->free(); - close_unlinked_files(deleted); if (fts_exist) { purge_sys.resume_FTS(); } @@ -11199,9 +11208,8 @@ foreign_fail: #endif } - row_mysql_unlock_data_dictionary(trx); + unlock_and_close_files(deleted, trx); trx->free(); - close_unlinked_files(deleted); if (fts_exist) { purge_sys.resume_FTS(); } -- cgit v1.2.1