From 59caf2c3c1fe128d1d2c3a8df9fadd4d25ab7102 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 21 Aug 2017 18:56:46 +0300 Subject: [PATCH] MDEV-13485 MTR tests fail massively with --innodb-sync-debug The parameter --innodb-sync-debug, which is disabled by default, aims to find potential deadlocks in InnoDB. When the parameter is enabled, lots of tests failed. Most of these failures were due to bogus diagnostics. But, as part of this fix, we are also fixing a bug in error handling code and removing dead code, and fixing cases where an uninitialized mutex was being locked and unlocked. dict_create_foreign_constraints_low(): Remove an extraneous mutex_exit() call that could cause corruption in an error handling path. Also, do not unnecessarily acquire dict_foreign_err_mutex. Its only purpose is to control concurrent access to dict_foreign_err_file. row_ins_foreign_trx_print(): Replace a redundant condition with a debug assertion. srv_dict_tmpfile, srv_dict_tmpfile_mutex: Remove. The temporary file is never being written to or read from. log_free_check(): Allow SYNC_FTS_CACHE (fts_cache_t::lock) to be held. ha_innobase::inplace_alter_table(), row_merge_insert_index_tuples(): Assert that no unexpected latches are being held. sync_latch_meta_init(): Properly initialize dict_operation_lock_key at SYNC_DICT_OPERATION. dict_sys->mutex is SYNC_DICT, and the now-removed SRV_DICT_TMPFILE was wrongly registered at SYNC_DICT_OPERATION. buf_block_init(): Correctly register buf_block_t::debug_latch. It was previously misleadingly reported as LATCH_ID_DICT_FOREIGN_ERR. latch_level_t: Correct the relative latching order of SYNC_IBUF_PESS_INSERT_MUTEX,SYNC_INDEX_TREE and SYNC_FILE_FORMAT_TAG,SYNC_DICT_OPERATION to avoid bogus failures. row_drop_table_for_mysql(): Avoid accessing btr_defragment_mutex if the defragmentation thread has not been started. This is the case during fts_drop_orphaned_tables() in recv_recovery_rollback_active(). fil_space_destroy_crypt_data(): Avoid acquiring fil_crypt_threads_mutex when it is uninitialized. We may have created crypt_data before the mutex was created, and the mutex creation would be skipped if InnoDB startup failed or --innodb-read-only was specified. --- mysql-test/suite/innodb/r/temporary_table.result | 2 +- storage/innobase/buf/buf0buf.cc | 10 ++++------ storage/innobase/dict/dict0dict.cc | 3 +-- storage/innobase/fil/fil0crypt.cc | 15 +++++++++++---- storage/innobase/handler/ha_innodb.cc | 1 - storage/innobase/handler/handler0alter.cc | 1 + storage/innobase/include/log0log.ic | 1 + storage/innobase/include/srv0srv.h | 6 ------ storage/innobase/include/sync0policy.h | 2 +- storage/innobase/include/sync0sync.h | 1 - storage/innobase/include/sync0types.h | 7 +++---- storage/innobase/row/row0ins.cc | 4 +--- storage/innobase/row/row0merge.cc | 9 +++++++++ storage/innobase/row/row0mysql.cc | 8 +++++++- storage/innobase/srv/srv0srv.cc | 6 ------ storage/innobase/srv/srv0start.cc | 15 --------------- storage/innobase/sync/sync0debug.cc | 8 +++----- storage/innobase/sync/sync0sync.cc | 1 - 18 files changed, 43 insertions(+), 57 deletions(-) diff --git a/mysql-test/suite/innodb/r/temporary_table.result b/mysql-test/suite/innodb/r/temporary_table.result index a6f2d28fc4a..310741b5798 100644 --- a/mysql-test/suite/innodb/r/temporary_table.result +++ b/mysql-test/suite/innodb/r/temporary_table.result @@ -137,7 +137,7 @@ Tables_in_test create temporary table t1 (keyc int, c1 char(100), c2 char(100)) engine = innodb; ERROR HY000: Can't create table `test`.`t1` (errno: 165 "Table is read only") # test various bad start-up parameters -FOUND 3 /InnoDB: Unable to create temporary file/ in mysqld.1.err +FOUND 2 /InnoDB: Unable to create temporary file/ in mysqld.1.err FOUND 1 /innodb_temporary and innodb_system file names seem to be the same/ in mysqld.1.err SELECT * FROM INFORMATION_SCHEMA.ENGINES WHERE engine = 'innodb' AND support IN ('YES', 'DEFAULT', 'ENABLED'); diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc index b57fba75869..ad93238410e 100644 --- a/storage/innobase/buf/buf0buf.cc +++ b/storage/innobase/buf/buf0buf.cc @@ -1477,17 +1477,15 @@ buf_block_init( rw_lock_create(PFS_NOT_INSTRUMENTED, &block->lock, SYNC_LEVEL_VARYING); - ut_d(rw_lock_create( - PFS_NOT_INSTRUMENTED, - &block->debug_latch, SYNC_NO_ORDER_CHECK)); + ut_d(rw_lock_create(PFS_NOT_INSTRUMENTED, &block->debug_latch, + SYNC_LEVEL_VARYING)); #else /* PFS_SKIP_BUFFER_MUTEX_RWLOCK || PFS_GROUP_BUFFER_SYNC */ rw_lock_create(buf_block_lock_key, &block->lock, SYNC_LEVEL_VARYING); - ut_d(rw_lock_create( - buf_block_debug_latch_key, - &block->debug_latch, SYNC_NO_ORDER_CHECK)); + ut_d(rw_lock_create(buf_block_debug_latch_key, + &block->debug_latch, SYNC_LEVEL_VARYING)); #endif /* PFS_SKIP_BUFFER_MUTEX_RWLOCK || PFS_GROUP_BUFFER_SYNC */ diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 0da37c8d6f5..146f3a15151 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -4600,7 +4600,6 @@ dict_create_foreign_constraints_low( if (!success) { ib::error() << "Could not find the table " << create_name << " being" << operation << " near to " << orig; - mutex_exit(&dict_foreign_err_mutex); ib_push_warning(trx, DB_ERROR, "%s table %s with foreign key constraint" @@ -5278,6 +5277,7 @@ try_find_index: " failed. You have more than one on delete or on update clause" " in '%s' near '%s'.\n", operation, create_name, start_of_latest_foreign, start_of_latest_set); + mutex_exit(&dict_foreign_err_mutex); ib_push_warning(trx, DB_CANNOT_ADD_CONSTRAINT, "%s table %s with foreign key constraint" @@ -5286,7 +5286,6 @@ try_find_index: operation, create_name, start_of_latest_foreign, start_of_latest_set); dict_foreign_free(foreign); - mutex_exit(&dict_foreign_err_mutex); return(DB_CANNOT_ADD_CONSTRAINT); } diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc index 366b9ef74ce..a2ce408619d 100644 --- a/storage/innobase/fil/fil0crypt.cc +++ b/storage/innobase/fil/fil0crypt.cc @@ -333,10 +333,17 @@ fil_space_destroy_crypt_data( fil_space_crypt_t **crypt_data) { if (crypt_data != NULL && (*crypt_data) != NULL) { - mutex_enter(&fil_crypt_threads_mutex); - fil_space_crypt_t* c = *crypt_data; - *crypt_data = NULL; - mutex_exit(&fil_crypt_threads_mutex); + fil_space_crypt_t* c; + if (UNIV_LIKELY(fil_crypt_threads_inited)) { + mutex_enter(&fil_crypt_threads_mutex); + c = *crypt_data; + *crypt_data = NULL; + mutex_exit(&fil_crypt_threads_mutex); + } else { + ut_ad(srv_read_only_mode || !srv_was_started); + c = *crypt_data; + *crypt_data = NULL; + } if (c) { c->~fil_space_crypt_t(); ut_free(c); diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 79d0c7543b0..f0fd67baea2 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -632,7 +632,6 @@ static PSI_mutex_info all_innodb_mutexes[] = { # endif /* UNIV_DEBUG */ PSI_KEY(rw_lock_list_mutex), PSI_KEY(rw_lock_mutex), - PSI_KEY(srv_dict_tmpfile_mutex), PSI_KEY(srv_innodb_monitor_mutex), PSI_KEY(srv_misc_tmpfile_mutex), PSI_KEY(srv_monitor_file_mutex), diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index ba4a4a2154f..3bcf4f710f6 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -6380,6 +6380,7 @@ ha_innobase::inplace_alter_table( DBUG_ENTER("inplace_alter_table"); DBUG_ASSERT(!srv_read_only_mode); + ut_ad(!sync_check_iterate(sync_check())); ut_ad(!rw_lock_own(dict_operation_lock, RW_LOCK_X)); ut_ad(!rw_lock_own(dict_operation_lock, RW_LOCK_S)); diff --git a/storage/innobase/include/log0log.ic b/storage/innobase/include/log0log.ic index f743985147c..58da7bacc6f 100644 --- a/storage/innobase/include/log0log.ic +++ b/storage/innobase/include/log0log.ic @@ -489,6 +489,7 @@ log_free_check(void) commit_try_rebuild() */ SYNC_DICT_OPERATION, /* dict_operation_lock X-latch during commit_try_rebuild() */ + SYNC_FTS_CACHE, /* fts_cache_t::lock */ SYNC_INDEX_TREE /* index->lock */ }; #endif /* UNIV_DEBUG */ diff --git a/storage/innobase/include/srv0srv.h b/storage/innobase/include/srv0srv.h index 2fc285270e8..3eddd300acc 100644 --- a/storage/innobase/include/srv0srv.h +++ b/storage/innobase/include/srv0srv.h @@ -227,12 +227,6 @@ extern ib_mutex_t page_zip_stat_per_index_mutex; extern ib_mutex_t srv_monitor_file_mutex; /* Temporary file for innodb monitor output */ extern FILE* srv_monitor_file; -/* Mutex for locking srv_dict_tmpfile. Only created if !srv_read_only_mode. -This mutex has a very high rank; threads reserving it should not -be holding any InnoDB latches. */ -extern ib_mutex_t srv_dict_tmpfile_mutex; -/* Temporary file for output from the data dictionary */ -extern FILE* srv_dict_tmpfile; /* Mutex for locking srv_misc_tmpfile. Only created if !srv_read_only_mode. This mutex has a very low rank; threads reserving it should not acquire any further latches or sleep before releasing this one. */ diff --git a/storage/innobase/include/sync0policy.h b/storage/innobase/include/sync0policy.h index 410e46f9c68..1b86d2633bf 100644 --- a/storage/innobase/include/sync0policy.h +++ b/storage/innobase/include/sync0policy.h @@ -61,7 +61,7 @@ public: : latch_t(id) { - /* No op */ + ut_ad(id != LATCH_ID_NONE); } /** Set to locked state diff --git a/storage/innobase/include/sync0sync.h b/storage/innobase/include/sync0sync.h index 7157b07e9d0..55aaf5032e8 100644 --- a/storage/innobase/include/sync0sync.h +++ b/storage/innobase/include/sync0sync.h @@ -91,7 +91,6 @@ extern mysql_pfs_key_t rw_lock_debug_mutex_key; # endif /* UNIV_DEBUG */ extern mysql_pfs_key_t rw_lock_list_mutex_key; extern mysql_pfs_key_t rw_lock_mutex_key; -extern mysql_pfs_key_t srv_dict_tmpfile_mutex_key; extern mysql_pfs_key_t srv_innodb_monitor_mutex_key; extern mysql_pfs_key_t srv_misc_tmpfile_mutex_key; extern mysql_pfs_key_t srv_monitor_file_mutex_key; diff --git a/storage/innobase/include/sync0types.h b/storage/innobase/include/sync0types.h index bcbcf70bfc7..8d08416cccd 100644 --- a/storage/innobase/include/sync0types.h +++ b/storage/innobase/include/sync0types.h @@ -260,9 +260,9 @@ enum latch_level_t { SYNC_TREE_NODE, SYNC_TREE_NODE_FROM_HASH, SYNC_TREE_NODE_NEW, + SYNC_IBUF_PESS_INSERT_MUTEX, SYNC_INDEX_TREE, - SYNC_IBUF_PESS_INSERT_MUTEX, SYNC_IBUF_HEADER, SYNC_DICT_HEADER, SYNC_STATS_AUTO_RECALC, @@ -270,10 +270,10 @@ enum latch_level_t { SYNC_DICT, SYNC_FTS_CACHE, - SYNC_DICT_OPERATION, - SYNC_FILE_FORMAT_TAG, + SYNC_DICT_OPERATION, + SYNC_TRX_I_S_LAST_READ, SYNC_TRX_I_S_RWLOCK, @@ -335,7 +335,6 @@ enum latch_id_t { LATCH_ID_RTR_PATH_MUTEX, LATCH_ID_RW_LOCK_LIST, LATCH_ID_RW_LOCK_MUTEX, - LATCH_ID_SRV_DICT_TMPFILE, LATCH_ID_SRV_INNODB_MONITOR, LATCH_ID_SRV_MISC_TMPFILE, LATCH_ID_SRV_MONITOR_FILE, diff --git a/storage/innobase/row/row0ins.cc b/storage/innobase/row/row0ins.cc index 16eec9f911f..936b96d5176 100644 --- a/storage/innobase/row/row0ins.cc +++ b/storage/innobase/row/row0ins.cc @@ -770,9 +770,7 @@ row_ins_foreign_trx_print( ulint n_trx_locks; ulint heap_size; - if (srv_read_only_mode) { - return; - } + ut_ad(!srv_read_only_mode); lock_mutex_enter(); n_rec_locks = lock_number_of_rows_locked(&trx->lock); diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/row0merge.cc index a6bf77835c9..cba453ced24 100644 --- a/storage/innobase/row/row0merge.cc +++ b/storage/innobase/row/row0merge.cc @@ -3618,7 +3618,16 @@ row_merge_insert_index_tuples( dtuple, tuple_heap); } +#ifdef UNIV_DEBUG + static const latch_level_t latches[] = { + SYNC_INDEX_TREE, /* index->lock */ + SYNC_LEVEL_VARYING /* btr_bulk->m_page_bulks */ + }; +#endif /* UNIV_DEBUG */ + ut_ad(dtuple_validate(dtuple)); + ut_ad(!sync_check_iterate(sync_allowed_latches(latches, + latches + 2))); error = btr_bulk->insert(dtuple); if (error != DB_SUCCESS) { diff --git a/storage/innobase/row/row0mysql.cc b/storage/innobase/row/row0mysql.cc index 720140e572e..db7f906b0cb 100644 --- a/storage/innobase/row/row0mysql.cc +++ b/storage/innobase/row/row0mysql.cc @@ -3666,7 +3666,13 @@ row_drop_table_for_mysql( dict_stats_recalc_pool_del(table); dict_stats_defrag_pool_del(table, NULL); - btr_defragment_remove_table(table); + if (btr_defragment_thread_active) { + /* During fts_drop_orphaned_tables() in + recv_recovery_rollback_active() the + btr_defragment_mutex has not yet been + initialized by btr_defragment_init(). */ + btr_defragment_remove_table(table); + } /* Remove stats for this table and all of its indexes from the persistent storage if it exists and if there are stats for this diff --git a/storage/innobase/srv/srv0srv.cc b/storage/innobase/srv/srv0srv.cc index c7162144be4..663487fc3a6 100644 --- a/storage/innobase/srv/srv0srv.cc +++ b/storage/innobase/srv/srv0srv.cc @@ -478,12 +478,6 @@ ib_mutex_t srv_monitor_file_mutex; /** Temporary file for innodb monitor output */ FILE* srv_monitor_file; -/** Mutex for locking srv_dict_tmpfile. Not created if srv_read_only_mode. -This mutex has a very high rank; threads reserving it should not -be holding any InnoDB latches. */ -ib_mutex_t srv_dict_tmpfile_mutex; -/** Temporary file for output from the data dictionary */ -FILE* srv_dict_tmpfile; /** Mutex for locking srv_misc_tmpfile. Not created if srv_read_only_mode. This mutex has a very low rank; threads reserving it should not acquire any further latches or sleep before releasing this one. */ diff --git a/storage/innobase/srv/srv0start.cc b/storage/innobase/srv/srv0start.cc index f0b29759423..d4922e33ef5 100644 --- a/storage/innobase/srv/srv0start.cc +++ b/storage/innobase/srv/srv0start.cc @@ -1764,15 +1764,6 @@ innobase_start_or_create_for_mysql() } } - mutex_create(LATCH_ID_SRV_DICT_TMPFILE, - &srv_dict_tmpfile_mutex); - - srv_dict_tmpfile = os_file_create_tmpfile(NULL); - - if (!srv_dict_tmpfile && err == DB_SUCCESS) { - err = DB_ERROR; - } - mutex_create(LATCH_ID_SRV_MISC_TMPFILE, &srv_misc_tmpfile_mutex); @@ -2847,11 +2838,6 @@ innodb_shutdown() } } - if (srv_dict_tmpfile) { - fclose(srv_dict_tmpfile); - srv_dict_tmpfile = 0; - } - if (srv_misc_tmpfile) { fclose(srv_misc_tmpfile); srv_misc_tmpfile = 0; @@ -2916,7 +2902,6 @@ innodb_shutdown() the temp files that the cover. */ if (!srv_read_only_mode) { mutex_free(&srv_monitor_file_mutex); - mutex_free(&srv_dict_tmpfile_mutex); mutex_free(&srv_misc_tmpfile_mutex); } diff --git a/storage/innobase/sync/sync0debug.cc b/storage/innobase/sync/sync0debug.cc index d6f3ef6c986..c80ea6aef3e 100644 --- a/storage/innobase/sync/sync0debug.cc +++ b/storage/innobase/sync/sync0debug.cc @@ -1431,9 +1431,6 @@ sync_latch_meta_init() LATCH_ADD_MUTEX(RW_LOCK_MUTEX, SYNC_NO_ORDER_CHECK, rw_lock_mutex_key); - LATCH_ADD_MUTEX(SRV_DICT_TMPFILE, SYNC_DICT_OPERATION, - srv_dict_tmpfile_mutex_key); - LATCH_ADD_MUTEX(SRV_INNODB_MONITOR, SYNC_NO_ORDER_CHECK, srv_innodb_monitor_mutex_key); @@ -1518,11 +1515,12 @@ sync_latch_meta_init() buf_block_lock_key); #ifdef UNIV_DEBUG - LATCH_ADD_RWLOCK(BUF_BLOCK_DEBUG, SYNC_NO_ORDER_CHECK, + LATCH_ADD_RWLOCK(BUF_BLOCK_DEBUG, SYNC_LEVEL_VARYING, buf_block_debug_latch_key); #endif /* UNIV_DEBUG */ - LATCH_ADD_RWLOCK(DICT_OPERATION, SYNC_DICT, dict_operation_lock_key); + LATCH_ADD_RWLOCK(DICT_OPERATION, SYNC_DICT_OPERATION, + dict_operation_lock_key); LATCH_ADD_RWLOCK(CHECKPOINT, SYNC_NO_ORDER_CHECK, checkpoint_lock_key); diff --git a/storage/innobase/sync/sync0sync.cc b/storage/innobase/sync/sync0sync.cc index 099a56c5457..4be7162f631 100644 --- a/storage/innobase/sync/sync0sync.cc +++ b/storage/innobase/sync/sync0sync.cc @@ -78,7 +78,6 @@ mysql_pfs_key_t rtr_path_mutex_key; mysql_pfs_key_t rtr_ssn_mutex_key; mysql_pfs_key_t rw_lock_list_mutex_key; mysql_pfs_key_t rw_lock_mutex_key; -mysql_pfs_key_t srv_dict_tmpfile_mutex_key; mysql_pfs_key_t srv_innodb_monitor_mutex_key; mysql_pfs_key_t srv_misc_tmpfile_mutex_key; mysql_pfs_key_t srv_monitor_file_mutex_key;