From 8724320989a53dd7f5ec13f169a6a722b9ec995d Mon Sep 17 00:00:00 2001 From: Jon Olav Hauglid <jon.hauglid@sun.com> Date: Thu, 10 Dec 2009 15:09:00 +0100 Subject: [PATCH] Backport of revno: 3685 Bug #48210 FLUSH TABLES WITH READ LOCK deadlocks against concurrent CREATE PROCEDURE This deadlock occured between a) CREATE PROCEDURE (or other commands listed below) b) FLUSH TABLES WITH READ LOCK If the execution of them happened in the following order: - a) opens a table (e.g. mysql.proc) - b) locks the global read lock (or GRL) - a) sleeps inside wait_if_global_read_lock() - b) increases refresh_version and sleeps waiting for old tables to go away Note that a) must start waiting on the GRL before FLUSH increases refresh_version. Otherwise a) won't wait on the GRL and instead close its tables for reopen, allowing FLUSH to complete and thus avoid the deadlock. With this patch the deadlock is avoided by making CREATE PROCEDURE acquire a protection against global read locks before it starts executing. This means that FLUSH TABLES WITH READ LOCK will have to wait until CREATE PROCEDURE completes before acquiring the global read lock, thereby avoiding the deadlock. This is implemented by introducing a new SQL command flag called CF_PROTECT_AGAINST_GRL. Commands marked with this flag will acquire a GRL protection in the beginning of mysql_execute_command(). This patch adds the flag to CREATE, ALTER and DROP for PROCEDURE and FUNCTION, as well as CREATE USER, DROP USER, RENAME USER and REVOKE ALL. All these commands either call open_grant_tables() or open_system_table_for_updated() which make them susceptible for this deadlock. The patch also adds the CF_PROTECT_AGAINST_GRL flag to a number of commands that previously acquired GRL protection in their respective SQLCOM case in mysql_execute_command(). Test case that checks for GRL protection for CREATE PROCEDURE and CREATE USER added to mdl_sync.test. --- mysql-test/r/mdl_sync.result | 37 ++++++++++++++ mysql-test/t/mdl_sync.test | 69 +++++++++++++++++++++++++ sql/lock.cc | 27 +++++++++- sql/sql_class.h | 10 ++++ sql/sql_parse.cc | 97 ++++++++++++------------------------ 5 files changed, 175 insertions(+), 65 deletions(-) diff --git a/mysql-test/r/mdl_sync.result b/mysql-test/r/mdl_sync.result index e5447c32b7d..130a8f22710 100644 --- a/mysql-test/r/mdl_sync.result +++ b/mysql-test/r/mdl_sync.result @@ -254,3 +254,40 @@ commit; # Switching to connection 'default'. # Clean-up drop table t1; +# +# Bug#48210 FLUSH TABLES WITH READ LOCK deadlocks +# against concurrent CREATE PROCEDURE +# +# Test 1: CREATE PROCEDURE +# Connection 1 +# Start CREATE PROCEDURE and open mysql.proc +SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait'; +CREATE PROCEDURE p1() SELECT 1; +# Connection 2 +SET DEBUG_SYNC= 'now WAIT_FOR table_opened'; +# Check that FLUSH must wait to get the GRL +# and let CREATE PROCEDURE continue +SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait'; +FLUSH TABLES WITH READ LOCK; +# Connection 1 +# Connection 2 +UNLOCK TABLES; +# Connection 1 +DROP PROCEDURE p1; +SET DEBUG_SYNC= 'RESET'; +# Test 2: CREATE USER +# Start CREATE USER and open the grant tables +SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait'; +CREATE USER 'user_1@localhost'; +# Connection 2 +SET DEBUG_SYNC= 'now WAIT_FOR table_opened'; +# Check that FLUSH must wait to get the GRL +# and let CREATE USER continue +SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait'; +FLUSH TABLES WITH READ LOCK; +# Connection 1 +# Connection 2 +UNLOCK TABLES; +# Connection 1 +DROP USER 'user_1@localhost'; +SET DEBUG_SYNC= 'RESET'; diff --git a/mysql-test/t/mdl_sync.test b/mysql-test/t/mdl_sync.test index fd66f6d539d..9422c67cb6f 100644 --- a/mysql-test/t/mdl_sync.test +++ b/mysql-test/t/mdl_sync.test @@ -475,6 +475,75 @@ disconnect con46673; drop table t1; +--echo # +--echo # Bug#48210 FLUSH TABLES WITH READ LOCK deadlocks +--echo # against concurrent CREATE PROCEDURE +--echo # + +connect (con2, localhost, root); + +--echo # Test 1: CREATE PROCEDURE + +--echo # Connection 1 +connection default; +--echo # Start CREATE PROCEDURE and open mysql.proc +SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait'; +--send CREATE PROCEDURE p1() SELECT 1 + +--echo # Connection 2 +connection con2; +SET DEBUG_SYNC= 'now WAIT_FOR table_opened'; +--echo # Check that FLUSH must wait to get the GRL +--echo # and let CREATE PROCEDURE continue +SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait'; +--send FLUSH TABLES WITH READ LOCK + +--echo # Connection 1 +connection default; +--reap + +--echo # Connection 2 +connection con2; +--reap +UNLOCK TABLES; + +--echo # Connection 1 +connection default; +DROP PROCEDURE p1; +SET DEBUG_SYNC= 'RESET'; + +--echo # Test 2: CREATE USER + +connection default; +--echo # Start CREATE USER and open the grant tables +SET DEBUG_SYNC= 'after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR grlwait'; +--send CREATE USER 'user_1@localhost' + +--echo # Connection 2 +connection con2; +SET DEBUG_SYNC= 'now WAIT_FOR table_opened'; +--echo # Check that FLUSH must wait to get the GRL +--echo # and let CREATE USER continue +SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL grlwait'; +--send FLUSH TABLES WITH READ LOCK + +--echo # Connection 1 +connection default; +--reap + +--echo # Connection 2 +connection con2; +--reap +UNLOCK TABLES; + +--echo # Connection 1 +connection default; +DROP USER 'user_1@localhost'; +SET DEBUG_SYNC= 'RESET'; + +disconnect con2; + + # Check that all connections opened by test cases in this file are really # gone so execution of other tests won't be affected by their presence. --source include/wait_until_count_sessions.inc diff --git a/sql/lock.cc b/sql/lock.cc index b64f7b0b44c..98217254240 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -74,6 +74,7 @@ */ #include "mysql_priv.h" +#include "debug_sync.h" #include <hash.h> #include <assert.h> @@ -1119,9 +1120,33 @@ bool lock_global_read_lock(THD *thd) if (!thd->global_read_lock) { const char *old_message; + const char *new_message= "Waiting to get readlock"; (void) pthread_mutex_lock(&LOCK_global_read_lock); + +#if defined(ENABLED_DEBUG_SYNC) + /* + The below sync point fires if we have to wait for + protect_against_global_read_lock. + + WARNING: Beware to use WAIT_FOR with this sync point. We hold + LOCK_global_read_lock here. + + Call the sync point before calling enter_cond() as it does use + enter_cond() and exit_cond() itself if a WAIT_FOR action is + executed in spite of the above warning. + + Pre-set proc_info so that it is available immediately after the + sync point sends a SIGNAL. This makes tests more reliable. + */ + if (protect_against_global_read_lock) + { + thd_proc_info(thd, new_message); + DEBUG_SYNC(thd, "wait_lock_global_read_lock"); + } +#endif /* defined(ENABLED_DEBUG_SYNC) */ + old_message=thd->enter_cond(&COND_global_read_lock, &LOCK_global_read_lock, - "Waiting to get readlock"); + new_message); DBUG_PRINT("info", ("waiting_for: %d protect_against: %d", waiting_for_read_lock, protect_against_global_read_lock)); diff --git a/sql/sql_class.h b/sql/sql_class.h index 889d7c5472b..4bfd3e66438 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -3341,6 +3341,16 @@ class select_dumpvar :public select_result_interceptor { */ #define CF_DIAGNOSTIC_STMT (1U << 8) +/** + SQL statements that must be protected against impending global read lock + to prevent deadlock. This deadlock could otherwise happen if the statement + starts waiting for the GRL to go away inside mysql_lock_tables while at the + same time having "old" opened tables. The thread holding the GRL can be + waiting for these "old" opened tables to be closed, causing a deadlock + (FLUSH TABLES WITH READ LOCK). + */ +#define CF_PROTECT_AGAINST_GRL (1U << 10) + /* Bits in server_command_flags */ /** diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index e462556c133..289b6ce0da8 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -182,14 +182,15 @@ void init_update_queries(void) memset(sql_command_flags, 0, sizeof(sql_command_flags)); sql_command_flags[SQLCOM_CREATE_TABLE]= CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE | - CF_AUTO_COMMIT_TRANS; + CF_AUTO_COMMIT_TRANS | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_CREATE_INDEX]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_ALTER_TABLE]= CF_CHANGES_DATA | CF_WRITE_LOGS_COMMAND | - CF_AUTO_COMMIT_TRANS; + CF_AUTO_COMMIT_TRANS | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_TRUNCATE]= CF_CHANGES_DATA | CF_WRITE_LOGS_COMMAND | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_DROP_TABLE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; - sql_command_flags[SQLCOM_LOAD]= CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE; + sql_command_flags[SQLCOM_LOAD]= CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE | + CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_CREATE_DB]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_DROP_DB]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_RENAME_TABLE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS; @@ -207,17 +208,17 @@ void init_update_queries(void) sql_command_flags[SQLCOM_DROP_TRIGGER]= CF_AUTO_COMMIT_TRANS; sql_command_flags[SQLCOM_UPDATE]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | - CF_REEXECUTION_FRAGILE; + CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_UPDATE_MULTI]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | - CF_REEXECUTION_FRAGILE; + CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_INSERT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | - CF_REEXECUTION_FRAGILE; + CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_INSERT_SELECT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | - CF_REEXECUTION_FRAGILE; + CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_DELETE]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | - CF_REEXECUTION_FRAGILE; + CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_DELETE_MULTI]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | - CF_REEXECUTION_FRAGILE; + CF_REEXECUTION_FRAGILE | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_REPLACE]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | CF_REEXECUTION_FRAGILE; sql_command_flags[SQLCOM_REPLACE_SELECT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT | @@ -276,20 +277,21 @@ void init_update_queries(void) CF_REEXECUTION_FRAGILE); - sql_command_flags[SQLCOM_CREATE_USER]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_RENAME_USER]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_DROP_USER]= CF_CHANGES_DATA; + sql_command_flags[SQLCOM_CREATE_USER]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_RENAME_USER]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_DROP_USER]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_GRANT]= CF_CHANGES_DATA; sql_command_flags[SQLCOM_REVOKE]= CF_CHANGES_DATA; + sql_command_flags[SQLCOM_REVOKE_ALL]= CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_ALTER_DB]= CF_CHANGES_DATA; sql_command_flags[SQLCOM_CREATE_FUNCTION]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_DROP_FUNCTION]= CF_CHANGES_DATA; + sql_command_flags[SQLCOM_DROP_FUNCTION]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_OPTIMIZE]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_CREATE_PROCEDURE]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_CREATE_SPFUNCTION]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_DROP_PROCEDURE]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_ALTER_PROCEDURE]= CF_CHANGES_DATA; - sql_command_flags[SQLCOM_ALTER_FUNCTION]= CF_CHANGES_DATA; + sql_command_flags[SQLCOM_CREATE_PROCEDURE]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_CREATE_SPFUNCTION]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_DROP_PROCEDURE]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_ALTER_PROCEDURE]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; + sql_command_flags[SQLCOM_ALTER_FUNCTION]= CF_CHANGES_DATA | CF_PROTECT_AGAINST_GRL; sql_command_flags[SQLCOM_INSTALL_PLUGIN]= CF_CHANGES_DATA; sql_command_flags[SQLCOM_UNINSTALL_PLUGIN]= CF_CHANGES_DATA; @@ -1969,6 +1971,17 @@ mysql_execute_command(THD *thd) thd->mdl_context.release_all_locks(); } + /* + Check if this command needs protection against the global read lock + to avoid deadlock. See CF_PROTECT_AGAINST_GRL. + start_waiting_global_read_lock() is called at the end of + mysql_execute_command(). + */ + if (((sql_command_flags[lex->sql_command] & CF_PROTECT_AGAINST_GRL) != 0) && + !thd->locked_tables_mode) + if (wait_if_global_read_lock(thd, FALSE, TRUE)) + goto error; + switch (lex->sql_command) { case SQLCOM_SHOW_EVENTS: @@ -2309,12 +2322,9 @@ case SQLCOM_PREPARE: start_waiting_global_read_lock(). We protect the normal CREATE TABLE in the same way. That way we avoid that a new table is created during a global read lock. + Protection against grl is covered by the CF_PROTECT_AGAINST_GRL flag. */ - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - { - res= 1; - goto end_with_restore_list; - } + #ifdef WITH_PARTITION_STORAGE_ENGINE { partition_info *part_info= thd->lex->part_info; @@ -2617,12 +2627,6 @@ case SQLCOM_PREPARE: "INDEX DIRECTORY"); create_info.data_file_name= create_info.index_file_name= NULL; - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - { - res= 1; - break; - } - thd->enable_slow_log= opt_log_slow_admin_statements; res= mysql_alter_table(thd, select_lex->db, lex->name.str, &create_info, @@ -2852,8 +2856,6 @@ case SQLCOM_PREPARE: DBUG_ASSERT(first_table == all_tables && first_table != 0); if (update_precheck(thd, all_tables)) break; - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - goto error; DBUG_ASSERT(select_lex->offset_limit == 0); unit->set_limit(select_lex); MYSQL_UPDATE_START(thd->query()); @@ -2884,15 +2886,6 @@ case SQLCOM_PREPARE: else res= 0; - /* - Protection might have already been risen if its a fall through - from the SQLCOM_UPDATE case above. - */ - if (!thd->locked_tables_mode && - lex->sql_command == SQLCOM_UPDATE_MULTI && - wait_if_global_read_lock(thd, 0, 1)) - goto error; - res= mysql_multi_update_prepare(thd); #ifdef HAVE_REPLICATION @@ -2992,11 +2985,6 @@ case SQLCOM_PREPARE: if ((res= insert_precheck(thd, all_tables))) break; - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - { - res= 1; - break; - } MYSQL_INSERT_START(thd->query()); res= mysql_insert(thd, all_tables, lex->field_list, lex->many_values, lex->update_list, lex->value_list, @@ -3031,11 +3019,6 @@ case SQLCOM_PREPARE: unit->set_limit(select_lex); - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - { - res= 1; - break; - } if (!(res= open_and_lock_tables(thd, all_tables))) { MYSQL_INSERT_SELECT_START(thd->query()); @@ -3113,11 +3096,6 @@ case SQLCOM_PREPARE: DBUG_ASSERT(select_lex->offset_limit == 0); unit->set_limit(select_lex); - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - { - res= 1; - break; - } MYSQL_DELETE_START(thd->query()); res = mysql_delete(thd, all_tables, select_lex->where, &select_lex->order_list, @@ -3133,12 +3111,6 @@ case SQLCOM_PREPARE: (TABLE_LIST *)thd->lex->auxiliary_table_list.first; multi_delete *del_result; - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - { - res= 1; - break; - } - if ((res= multi_delete_precheck(thd, all_tables))) break; @@ -3277,9 +3249,6 @@ case SQLCOM_PREPARE: if (check_one_table_access(thd, privilege, all_tables)) goto error; - if (!thd->locked_tables_mode && wait_if_global_read_lock(thd, 0, 1)) - goto error; - res= mysql_load(thd, lex->exchange, first_table, lex->field_list, lex->update_list, lex->value_list, lex->duplicates, lex->ignore, (bool) lex->local_file); -- 2.30.9