diff --git a/mysql-test/main/lock_multi.result b/mysql-test/main/lock_multi.result index 1d9528764df..5441ba1ecb4 100644 --- a/mysql-test/main/lock_multi.result +++ b/mysql-test/main/lock_multi.result @@ -218,6 +218,7 @@ connection con2; unlock tables; connection con3; a +connection con4; connection default; disconnect con5; disconnect con4; @@ -247,6 +248,7 @@ flush table t2; connection default; unlock tables; connection con1; +connection con2; # # LOCK TABLES .. WRITE # @@ -299,7 +301,7 @@ connection default; alter table t1 add column j int; connect insert,localhost,root,,test,,; connection insert; -insert into t1 values (1,2);; +insert into t1 values (1,2); connection default; unlock tables; connection flush; diff --git a/mysql-test/main/lock_multi.test b/mysql-test/main/lock_multi.test index 16845aa8acb..59c6978cb5a 100644 --- a/mysql-test/main/lock_multi.test +++ b/mysql-test/main/lock_multi.test @@ -392,8 +392,13 @@ let $wait_condition= --echo # waiting for release of read lock connection con4; --echo # would hang and later cause a deadlock -flush tables t2; +--send flush tables t2 connection con1; +let $wait_condition= + select count(*) = 1 from information_schema.processlist + where state = "Waiting for table metadata lock" and + info = "flush tables t2"; +--source include/wait_condition.inc --echo # clean up unlock tables; connection con2; @@ -401,6 +406,8 @@ connection con2; unlock tables; connection con3; --reap +connection con4; +--reap connection default; disconnect con5; disconnect con4; @@ -437,11 +444,18 @@ let $wait_condition= --source include/wait_condition.inc --echo # statement is waiting for release of read lock connection con2; -flush table t2; +--send flush table t2 connection default; +let $wait_condition= + select count(*) = 1 from information_schema.processlist + where state = "Waiting for table metadata lock" and + info = "flush table t2"; +--source include/wait_condition.inc unlock tables; connection con1; --reap +connection con2; +--reap --echo # --echo # LOCK TABLES .. WRITE @@ -553,11 +567,11 @@ let $wait_condition= where state = "Waiting for backup lock" and info = "flush tables with read lock"; --source include/wait_condition.inc ---send insert into t1 values (1,2); +--send insert into t1 values (1,2) connection default; let $wait_condition= select count(*) = 1 from information_schema.processlist - where state = "Waiting for backup lock" and + where state = "Waiting for table metadata lock" and info = "insert into t1 values (1,2)"; --source include/wait_condition.inc unlock tables; diff --git a/mysql-test/main/mdl_sync.result b/mysql-test/main/mdl_sync.result index 7e90720dca5..f5268e557ab 100644 --- a/mysql-test/main/mdl_sync.result +++ b/mysql-test/main/mdl_sync.result @@ -3082,3 +3082,72 @@ connection default; SET debug_sync='RESET'; DROP TABLE t1; disconnect con1; +# +# MDEV-5336 - Implement LOCK FOR BACKUP +# +# Make sure deadlock detector prefers FTWRL connection as a victim +# and FTWRL retries lock attempt. This deadlock was present before +# MDEV-5336. +CREATE TABLE t1(a INT) ENGINE=InnoDB; +CREATE TABLE t2(a INT) ENGINE=InnoDB; +BEGIN; +SELECT * FROM t2; +a +# +connect con1,localhost,root,,; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +LOCK TABLES t2 WRITE; +# +connect con2,localhost,root,,; +SET DEBUG_SYNC='now WAIT_FOR waiting'; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +FLUSH TABLES WITH READ LOCK; +# +connection default; +SET DEBUG_SYNC='now WAIT_FOR waiting'; +INSERT INTO t1 VALUES(1); +COMMIT; +connection con1; +UNLOCK TABLES; +connection con2; +UNLOCK TABLES; +connection default; +DROP TABLE t1, t2; +SET DEBUG_SYNC='RESET'; +disconnect con1; +disconnect con2; +# Make sure deadlock detector prefers FTWRL connection as a victim +# and FTWRL retries lock attempt. This deadlock was found during +# MDEV-5336 review. +CREATE TABLE t1(a INT) ENGINE=InnoDB; +CREATE TABLE t2(a INT) ENGINE=InnoDB; +BEGIN; +INSERT INTO t2 VALUES(1); +SET DEBUG_SYNC='after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR go'; +INSERT INTO t1 VALUES(1); +# +connect con1,localhost,root,,; +SET DEBUG_SYNC='now WAIT_FOR table_opened'; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +LOCK TABLES t1 WRITE; +# +connect con2,localhost,root,,; +SET DEBUG_SYNC='now WAIT_FOR waiting'; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +FLUSH TABLES WITH READ LOCK; +# +connect con3,localhost,root,,; +SET DEBUG_SYNC='now WAIT_FOR waiting'; +SET DEBUG_SYNC='now SIGNAL go'; +connection default; +COMMIT; +connection con1; +UNLOCK TABLES; +connection con2; +UNLOCK TABLES; +connection default; +DROP TABLE t1, t2; +SET DEBUG_SYNC='RESET'; +disconnect con1; +disconnect con2; +disconnect con3; diff --git a/mysql-test/main/mdl_sync.test b/mysql-test/main/mdl_sync.test index 47eafa97a33..7c19eab5c37 100644 --- a/mysql-test/main/mdl_sync.test +++ b/mysql-test/main/mdl_sync.test @@ -4122,6 +4122,91 @@ DROP TABLE t1; disconnect con1; + +--echo # +--echo # MDEV-5336 - Implement LOCK FOR BACKUP +--echo # + +--echo # Make sure deadlock detector prefers FTWRL connection as a victim +--echo # and FTWRL retries lock attempt. This deadlock was present before +--echo # MDEV-5336. +CREATE TABLE t1(a INT) ENGINE=InnoDB; +CREATE TABLE t2(a INT) ENGINE=InnoDB; +BEGIN; +SELECT * FROM t2; + +--echo # +connect(con1,localhost,root,,); +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +send LOCK TABLES t2 WRITE; + +--echo # +connect(con2,localhost,root,,); +SET DEBUG_SYNC='now WAIT_FOR waiting'; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +send FLUSH TABLES WITH READ LOCK; + +--echo # +connection default; +SET DEBUG_SYNC='now WAIT_FOR waiting'; +INSERT INTO t1 VALUES(1); +COMMIT; + +connection con1; +reap; +UNLOCK TABLES; +connection con2; +reap; +UNLOCK TABLES; +connection default; +DROP TABLE t1, t2; +SET DEBUG_SYNC='RESET'; +disconnect con1; +disconnect con2; + +--echo # Make sure deadlock detector prefers FTWRL connection as a victim +--echo # and FTWRL retries lock attempt. This deadlock was found during +--echo # MDEV-5336 review. +CREATE TABLE t1(a INT) ENGINE=InnoDB; +CREATE TABLE t2(a INT) ENGINE=InnoDB; +BEGIN; +INSERT INTO t2 VALUES(1); +SET DEBUG_SYNC='after_open_table_mdl_shared SIGNAL table_opened WAIT_FOR go'; +send INSERT INTO t1 VALUES(1); + +--echo # +connect(con1,localhost,root,,); +SET DEBUG_SYNC='now WAIT_FOR table_opened'; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +send LOCK TABLES t1 WRITE; + +--echo # +connect(con2,localhost,root,,); +SET DEBUG_SYNC='now WAIT_FOR waiting'; +SET DEBUG_SYNC='mdl_acquire_lock_wait SIGNAL waiting'; +send FLUSH TABLES WITH READ LOCK; + +--echo # +connect(con3,localhost,root,,); +SET DEBUG_SYNC='now WAIT_FOR waiting'; +SET DEBUG_SYNC='now SIGNAL go'; + +connection default; +reap; +COMMIT; +connection con1; +reap; +UNLOCK TABLES; +connection con2; +reap; +UNLOCK TABLES; +connection default; +DROP TABLE t1, t2; +SET DEBUG_SYNC='RESET'; +disconnect con1; +disconnect con2; +disconnect con3; + # 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 70d1003e946..1103bc96f1a 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -952,10 +952,10 @@ bool lock_object_name(THD *thd, MDL_key::enum_mdl_namespace mdl_type, semi-automatic. We assume that any statement which should be blocked by global read lock will either open and acquires write-lock on tables or acquires metadata locks on objects it is going to modify. For any - such statement global IX metadata lock is automatically acquired for - its duration (in case of LOCK TABLES until end of LOCK TABLES mode). - And lock_global_read_lock() simply acquires global S metadata lock - and thus prohibits execution of statements which modify data (unless + such statement MDL_BACKUP_STMT metadata lock is automatically acquired + for its duration (in case of LOCK TABLES until end of LOCK TABLES mode). + And lock_global_read_lock() simply acquires MDL_BACKUP_FTWRL1 metadata + lock and thus prohibits execution of statements which modify data (unless they modify only temporary tables). If deadlock happens it is detected by MDL subsystem and resolved in the standard fashion (by backing-off metadata locks acquired so far and restarting open tables process @@ -998,6 +998,17 @@ bool lock_object_name(THD *thd, MDL_key::enum_mdl_namespace mdl_type, If the global read lock is already taken by this thread, then nothing is done. + Concurrent thread can acquire protection against global read lock either + before or after it got table metadata lock. This may lead to a deadlock if + there is pending global read lock request. E.g. + t1 does DML, holds SHARED table lock, waiting for t3 (GRL protection) + t2 does DDL, holds GRL protection, waiting for t1 (EXCLUSIVE) + t3 does FTWRL, has pending GRL, waiting for t2 (GRL) + + Since this is very seldom deadlock and FTWRL connection must not hold any + other locks, FTWRL connection is made deadlock victim and attempt to acquire + GRL retried. + See also "Handling of global read locks" above. @param thd Reference to thread. @@ -1012,7 +1023,9 @@ bool Global_read_lock::lock_global_read_lock(THD *thd) if (!m_state) { + MDL_deadlock_and_lock_abort_error_handler mdl_deadlock_handler; MDL_request mdl_request; + bool result; mysql_ha_cleanup_no_free(thd); @@ -1022,9 +1035,17 @@ bool Global_read_lock::lock_global_read_lock(THD *thd) MDL_BACKUP_FTWRL2)); mdl_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_FTWRL1, MDL_EXPLICIT); - if (thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) - DBUG_RETURN(1); + do + { + mdl_deadlock_handler.init(); + thd->push_internal_handler(&mdl_deadlock_handler); + result= thd->mdl_context.acquire_lock(&mdl_request, + thd->variables.lock_wait_timeout); + thd->pop_internal_handler(); + } while (mdl_deadlock_handler.need_reopen()); + + if (result) + DBUG_RETURN(true); m_mdl_global_read_lock= mdl_request.ticket; m_state= GRL_ACQUIRED; diff --git a/sql/mdl.cc b/sql/mdl.cc index 8a946ad2de0..b096f944fb5 100644 --- a/sql/mdl.cc +++ b/sql/mdl.cc @@ -1015,9 +1015,14 @@ void MDL_ticket::destroy(MDL_ticket *ticket) uint MDL_ticket::get_deadlock_weight() const { - return (m_lock->key.mdl_namespace() == MDL_key::BACKUP || - m_type >= MDL_SHARED_UPGRADABLE ? - DEADLOCK_WEIGHT_DDL : DEADLOCK_WEIGHT_DML); + if (m_lock->key.mdl_namespace() == MDL_key::BACKUP) + { + if (m_type == MDL_BACKUP_FTWRL1) + return DEADLOCK_WEIGHT_FTWRL1; + return DEADLOCK_WEIGHT_DDL; + } + return m_type >= MDL_SHARED_UPGRADABLE ? + DEADLOCK_WEIGHT_DDL : DEADLOCK_WEIGHT_DML; } diff --git a/sql/mdl.h b/sql/mdl.h index 6b615b2192d..425c7a43ea6 100644 --- a/sql/mdl.h +++ b/sql/mdl.h @@ -591,7 +591,8 @@ public: enum enum_deadlock_weight { - DEADLOCK_WEIGHT_DML= 0, + DEADLOCK_WEIGHT_FTWRL1= 0, + DEADLOCK_WEIGHT_DML= 1, DEADLOCK_WEIGHT_DDL= 100 }; /* A helper used to determine which lock request should be aborted. */ diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 96570861122..fa5c4570694 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1829,59 +1829,6 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) if (! (flags & MYSQL_OPEN_HAS_MDL_LOCK)) { - /* - We are not under LOCK TABLES and going to acquire write-lock/ - modify the base table. We need to acquire protection against - global read lock until end of this statement in order to have - this statement blocked by active FLUSH TABLES WITH READ LOCK. - - We don't need to acquire this protection under LOCK TABLES as - such protection already acquired at LOCK TABLES time and - not released until UNLOCK TABLES. - - We don't block statements which modify only temporary tables - as these tables are not preserved by any form of - backup which uses FLUSH TABLES WITH READ LOCK. - - TODO: The fact that we sometimes acquire protection against - GRL only when we encounter table to be write-locked - slightly increases probability of deadlock. - This problem will be solved once Alik pushes his - temporary table refactoring patch and we can start - pre-acquiring metadata locks at the beggining of - open_tables() call. - */ - if (table_list->mdl_request.is_write_lock_request() && - ! (flags & (MYSQL_OPEN_IGNORE_GLOBAL_READ_LOCK | - MYSQL_OPEN_FORCE_SHARED_MDL | - MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL | - MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) && - ! ot_ctx->has_protection_against_grl()) - { - MDL_request protection_request; - MDL_deadlock_handler mdl_deadlock_handler(ot_ctx); - - if (thd->global_read_lock.can_acquire_protection()) - DBUG_RETURN(TRUE); - - protection_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_STMT, - MDL_STATEMENT); - - /* - Install error handler which if possible will convert deadlock error - into request to back-off and restart process of opening tables. - */ - thd->push_internal_handler(&mdl_deadlock_handler); - bool result= thd->mdl_context.acquire_lock(&protection_request, - ot_ctx->get_timeout()); - thd->pop_internal_handler(); - - if (result) - DBUG_RETURN(TRUE); - - ot_ctx->set_has_protection_against_grl(); - } - if (open_table_get_mdl_lock(thd, ot_ctx, &table_list->mdl_request, flags, &mdl_ticket) || mdl_ticket == NULL) @@ -2091,6 +2038,71 @@ retry_share: tc_add_table(thd, table); } + + if (!(flags & MYSQL_OPEN_HAS_MDL_LOCK)) + { + /* + We are not under LOCK TABLES and going to acquire write-lock/ + modify the base table. We need to acquire protection against + global read lock until end of this statement in order to have + this statement blocked by active FLUSH TABLES WITH READ LOCK. + + We don't need to acquire this protection under LOCK TABLES as + such protection already acquired at LOCK TABLES time and + not released until UNLOCK TABLES. + + We don't block statements which modify only temporary tables + as these tables are not preserved by any form of + backup which uses FLUSH TABLES WITH READ LOCK. + + TODO: The fact that we sometimes acquire protection against + GRL only when we encounter table to be write-locked + slightly increases probability of deadlock. + This problem will be solved once Alik pushes his + temporary table refactoring patch and we can start + pre-acquiring metadata locks at the beggining of + open_tables() call. + */ + if (table_list->mdl_request.is_write_lock_request() && + ! (flags & (MYSQL_OPEN_IGNORE_GLOBAL_READ_LOCK | + MYSQL_OPEN_FORCE_SHARED_MDL | + MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL | + MYSQL_OPEN_SKIP_SCOPED_MDL_LOCK)) && + ! ot_ctx->has_protection_against_grl()) + { + MDL_request protection_request; + MDL_deadlock_handler mdl_deadlock_handler(ot_ctx); + + if (thd->global_read_lock.can_acquire_protection()) + { + MYSQL_UNBIND_TABLE(table->file); + tc_release_table(table); + DBUG_RETURN(TRUE); + } + + protection_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_STMT, + MDL_STATEMENT); + + /* + Install error handler which if possible will convert deadlock error + into request to back-off and restart process of opening tables. + */ + thd->push_internal_handler(&mdl_deadlock_handler); + bool result= thd->mdl_context.acquire_lock(&protection_request, + ot_ctx->get_timeout()); + thd->pop_internal_handler(); + + if (result) + { + MYSQL_UNBIND_TABLE(table->file); + tc_release_table(table); + DBUG_RETURN(TRUE); + } + + ot_ctx->set_has_protection_against_grl(); + } + } + table->mdl_ticket= mdl_ticket; table->next= thd->open_tables; /* Link into simple list */