From eb0f09712e1b09955cc9e60d516ddf19336c9ffc Mon Sep 17 00:00:00 2001 From: Dmitry Lenev Date: Mon, 15 Feb 2010 13:23:34 +0300 Subject: [PATCH 1/2] Fix for bug #51134 "Crash in MDL_lock::destroy on a concurrent DDL workload". When a RENAME TABLE or LOCK TABLE ... WRITE statement which mentioned the same table several times were aborted during the process of acquring metadata locks (due to deadlock which was discovered or because of KILL statement) server might have crashed. When attempt to acquire all locks requested had failed we went through the list of requests and released locks which we have managed to acquire by that moment one by one. Since in the scenario described above list of requests contained duplicates this led to releasing the same ticket twice and a crash as result. This patch solves the problem by employing different approach to releasing locks in case of failure to acquire all locks requested. Now we take a MDL savepoint before starting acquiring locks and simply rollback to it if things go bad. mysql-test/r/lock_multi.result: Updated test results (see lock_multi.test). mysql-test/t/lock_multi.test: Added test case for bug #51134 "Crash in MDL_lock::destroy on a concurrent DDL workload". sql/mdl.cc: MDL_context::acquire_locks(): When attempt to acquire all locks requested has failed do not go through the list of requests and release locks which we have managed to acquire one by one. Since list of requests can contain duplicates such approach may lead to releasing the same ticket twice and a crash as result. Instead use the following approach - take a MDL savepoint before starting acquiring locks and simply rollback to it if things go bad. --- mysql-test/r/lock_multi.result | 26 ++++++++++++++++ mysql-test/t/lock_multi.test | 54 ++++++++++++++++++++++++++++++++++ sql/mdl.cc | 11 +++++-- 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/mysql-test/r/lock_multi.result b/mysql-test/r/lock_multi.result index d67868e927a..c440dafb228 100644 --- a/mysql-test/r/lock_multi.result +++ b/mysql-test/r/lock_multi.result @@ -436,3 +436,29 @@ UNLOCK TABLES; # Reaping: DROP TABLE t1, t2 # Connection default # Cleanup +# +# Test for bug #51134 "Crash in MDL_lock::destroy on a concurrent +# DDL workload". +# +drop tables if exists t1, t2, t3; +create table t3 (i int); +# Switching to connection 'con1' +# Lock 't3' so upcoming RENAME is blocked. +lock table t3 read; +# Switching to connection 'con2' +# Remember ID for this connection. +# Start statement which will try to acquire two instances +# of X metadata lock on the same object. +# Sending: +rename tables t1 to t2, t2 to t3;; +# Switching to connection 'default' +# Wait until RENAME TABLE is blocked on table 't3'. +# Kill RENAME TABLE. +kill query ID; +# Switching to connection 'con2' +# RENAME TABLE should be aborted but should not crash. +ERROR 70100: Query execution was interrupted +# Switching to connection 'con1' +unlock tables; +# Switching to connection 'default' +drop table t3; diff --git a/mysql-test/t/lock_multi.test b/mysql-test/t/lock_multi.test index 1080b44c448..c34dcb05dd4 100644 --- a/mysql-test/t/lock_multi.test +++ b/mysql-test/t/lock_multi.test @@ -1038,5 +1038,59 @@ disconnect con2; disconnect con3; +--echo # +--echo # Test for bug #51134 "Crash in MDL_lock::destroy on a concurrent +--echo # DDL workload". +--echo # +--disable_warnings +drop tables if exists t1, t2, t3; +--enable_warnings +connect (con1, localhost, root, , ); +connect (con2, localhost, root, , ); +connection default; +create table t3 (i int); + +--echo # Switching to connection 'con1' +connection con1; +--echo # Lock 't3' so upcoming RENAME is blocked. +lock table t3 read; + +--echo # Switching to connection 'con2' +connection con2; +--echo # Remember ID for this connection. +let $ID= `select connection_id()`; +--echo # Start statement which will try to acquire two instances +--echo # of X metadata lock on the same object. +--echo # Sending: +--send rename tables t1 to t2, t2 to t3; + +--echo # Switching to connection 'default' +connection default; +--echo # Wait until RENAME TABLE is blocked on table 't3'. +let $wait_condition= + select count(*) = 1 from information_schema.processlist + where state = "Waiting for table" and info = "rename tables t1 to t2, t2 to t3"; +--source include/wait_condition.inc +--echo # Kill RENAME TABLE. +--replace_result $ID ID +eval kill query $ID; + +--echo # Switching to connection 'con2' +connection con2; +--echo # RENAME TABLE should be aborted but should not crash. +--error ER_QUERY_INTERRUPTED +--reap + +--echo # Switching to connection 'con1' +connection con1; +unlock tables; + +--echo # Switching to connection 'default' +connection default; +disconnect con1; +disconnect con2; +drop table t3; + + # Wait till all disconnects are completed --source include/wait_until_count_sessions.inc diff --git a/sql/mdl.cc b/sql/mdl.cc index 245d6d8a018..e493b42ca69 100644 --- a/sql/mdl.cc +++ b/sql/mdl.cc @@ -1532,6 +1532,7 @@ bool MDL_context::acquire_locks(MDL_request_list *mdl_requests, { MDL_request_list::Iterator it(*mdl_requests); MDL_request **sort_buf, **p_req; + MDL_ticket *mdl_svp= mdl_savepoint(); ssize_t req_count= static_cast(mdl_requests->elements()); if (req_count == 0) @@ -1565,12 +1566,16 @@ bool MDL_context::acquire_locks(MDL_request_list *mdl_requests, return FALSE; err: - /* Release locks we have managed to acquire so far. */ + /* + Release locks we have managed to acquire so far. + Use rollback_to_savepoint() since there may be duplicate + requests that got assigned the same ticket. + */ + rollback_to_savepoint(mdl_svp); + /* Reset lock requests back to its initial state. */ for (req_count= p_req - sort_buf, p_req= sort_buf; p_req < sort_buf + req_count; p_req++) { - release_lock((*p_req)->ticket); - /* Reset lock request back to its initial state. */ (*p_req)->ticket= NULL; } my_free(sort_buf, MYF(0)); From be3e256d25a853545c6ee04a8bf8e5b5907889c9 Mon Sep 17 00:00:00 2001 From: Dmitry Lenev Date: Mon, 15 Feb 2010 14:23:36 +0300 Subject: [PATCH 2/2] Fix for bug #51136 "Crash in pthread_rwlock_rdlock on TEMPORARY + HANDLER + LOCK + SP". Server crashed when one: 1) Opened HANDLER or acquired global read lock 2) Then locked one or several temporary tables with LOCK TABLES statement (but no base tables). 3) Then issued any statement causing commit (explicit or implicit). 4) Issued statement which should have closed HANDLER or released global read lock. The problem was that when entering LOCK TABLES mode in the scenario described above we incorrectly set transactional MDL sentinel to zero. As result during commit all metadata locks were released (including lock for open HANDLER or global metadata shared lock). Indeed, attempt to release metadata lock for the second time which happened during HANLDER CLOSE or during release of GLR caused crash. This patch fixes problem by changing MDL_context's set_trans_sentinel() method to set sentinel to correct value (it should point to the most recent ticket). mysql-test/include/handler.inc: Added test for bug #51136 "Crash in pthread_rwlock_rdlock on TEMPORARY + HANDLER + LOCK + SP". mysql-test/r/flush.result: Updated test results (see flush.test). mysql-test/r/handler_innodb.result: Updated test results (see include/handler.inc). mysql-test/r/handler_myisam.result: Updated test results (see include/handler.inc). mysql-test/t/flush.test: Added additional coverage for bug #51136 "Crash in pthread_rwlock_rdlock on TEMPORARY + HANDLER + LOCK + SP". sql/mdl.h: When setting new value of transactional sentinel use pointer to the most recent ticket instead of value returned by MDL_context::mdl_savepoint(). This allows to handle correctly situation when the new value of sentinel should be the same as its current value (MDL_context::mdl_savepoint() returns NULL in this case). --- mysql-test/include/handler.inc | 22 ++++++++++++++++++++++ mysql-test/r/flush.result | 17 +++++++++++++++++ mysql-test/r/handler_innodb.result | 18 ++++++++++++++++++ mysql-test/r/handler_myisam.result | 18 ++++++++++++++++++ mysql-test/t/flush.test | 21 +++++++++++++++++++++ sql/mdl.h | 2 +- 6 files changed, 97 insertions(+), 1 deletion(-) diff --git a/mysql-test/include/handler.inc b/mysql-test/include/handler.inc index 9c6c6864e05..4b2e64e3bb5 100644 --- a/mysql-test/include/handler.inc +++ b/mysql-test/include/handler.inc @@ -1681,3 +1681,25 @@ handler t1 close; --echo # Clean-up. drop function f1; drop tables t1, t2; + + +--echo # +--echo # Test for bug #51136 "Crash in pthread_rwlock_rdlock on TEMPORARY + +--echo # HANDLER + LOCK + SP". +--echo # Also see additional coverage for this bug in flush.test. +--echo # +--disable_warnings +drop tables if exists t1, t2; +--enable_warnings +create table t1 (i int); +create temporary table t2 (j int); +handler t1 open; +lock table t2 read; +--echo # This commit should not release any MDL locks. +commit; +unlock tables; +--echo # The below statement crashed before the bug fix as it +--echo # has attempted to release metadata lock which was +--echo # already released by commit. +handler t1 close; +drop tables t1, t2; diff --git a/mysql-test/r/flush.result b/mysql-test/r/flush.result index 2be426d3a4a..2136bcd92f1 100644 --- a/mysql-test/r/flush.result +++ b/mysql-test/r/flush.result @@ -94,3 +94,20 @@ unlock tables; set global general_log= @old_general_log; set global read_only= @old_read_only; End of 5.1 tests +# +# Additional test for bug #51136 "Crash in pthread_rwlock_rdlock +# on TEMPORARY + HANDLER + LOCK + SP". +# Also see the main test for this bug in include/handler.inc. +# +drop tables if exists t1, t2; +create table t1 (i int); +create temporary table t2 (j int); +flush tables with read lock; +lock table t2 read; +# This commit should not release any MDL locks. +commit; +# The below statement crashed before the bug fix as it +# has attempted to release global shared metadata lock +# which was already released by commit. +unlock tables; +drop tables t1, t2; diff --git a/mysql-test/r/handler_innodb.result b/mysql-test/r/handler_innodb.result index d0d35590e73..58083194b83 100644 --- a/mysql-test/r/handler_innodb.result +++ b/mysql-test/r/handler_innodb.result @@ -1667,3 +1667,21 @@ handler t1 close; # Clean-up. drop function f1; drop tables t1, t2; +# +# Test for bug #51136 "Crash in pthread_rwlock_rdlock on TEMPORARY + +# HANDLER + LOCK + SP". +# Also see additional coverage for this bug in flush.test. +# +drop tables if exists t1, t2; +create table t1 (i int); +create temporary table t2 (j int); +handler t1 open; +lock table t2 read; +# This commit should not release any MDL locks. +commit; +unlock tables; +# The below statement crashed before the bug fix as it +# has attempted to release metadata lock which was +# already released by commit. +handler t1 close; +drop tables t1, t2; diff --git a/mysql-test/r/handler_myisam.result b/mysql-test/r/handler_myisam.result index 73ad8609376..dd199e40574 100644 --- a/mysql-test/r/handler_myisam.result +++ b/mysql-test/r/handler_myisam.result @@ -1664,6 +1664,24 @@ handler t1 close; drop function f1; drop tables t1, t2; # +# Test for bug #51136 "Crash in pthread_rwlock_rdlock on TEMPORARY + +# HANDLER + LOCK + SP". +# Also see additional coverage for this bug in flush.test. +# +drop tables if exists t1, t2; +create table t1 (i int); +create temporary table t2 (j int); +handler t1 open; +lock table t2 read; +# This commit should not release any MDL locks. +commit; +unlock tables; +# The below statement crashed before the bug fix as it +# has attempted to release metadata lock which was +# already released by commit. +handler t1 close; +drop tables t1, t2; +# # BUG #46456: HANDLER OPEN + TRUNCATE + DROP (temporary) TABLE, crash # CREATE TABLE t1 AS SELECT 1 AS f1; diff --git a/mysql-test/t/flush.test b/mysql-test/t/flush.test index 4172230a54d..d41ac3100b0 100644 --- a/mysql-test/t/flush.test +++ b/mysql-test/t/flush.test @@ -203,3 +203,24 @@ set global general_log= @old_general_log; set global read_only= @old_read_only; --echo End of 5.1 tests + + +--echo # +--echo # Additional test for bug #51136 "Crash in pthread_rwlock_rdlock +--echo # on TEMPORARY + HANDLER + LOCK + SP". +--echo # Also see the main test for this bug in include/handler.inc. +--echo # +--disable_warnings +drop tables if exists t1, t2; +--enable_warnings +create table t1 (i int); +create temporary table t2 (j int); +flush tables with read lock; +lock table t2 read; +--echo # This commit should not release any MDL locks. +commit; +--echo # The below statement crashed before the bug fix as it +--echo # has attempted to release global shared metadata lock +--echo # which was already released by commit. +unlock tables; +drop tables t1, t2; diff --git a/sql/mdl.h b/sql/mdl.h index d43548fb65f..59bc1f64762 100644 --- a/sql/mdl.h +++ b/sql/mdl.h @@ -501,7 +501,7 @@ public: void set_trans_sentinel() { - m_trans_sentinel= mdl_savepoint(); + m_trans_sentinel= m_tickets.front(); } MDL_ticket *trans_sentinel() const { return m_trans_sentinel; }