From e53e16a885937a902b10d4ab0293f3ec0905e355 Mon Sep 17 00:00:00 2001 From: Jon Olav Hauglid Date: Wed, 6 Oct 2010 09:56:29 +0200 Subject: [PATCH] Bug #57002 Assert in upgrade_shared_lock_to_exclusive() for ALTER TABLE + MERGE tables The patch for Bug#56292 changed how metadata locks are taken for MERGE tables. After the patch, locking the MERGE table will also lock the children tables with the same metadata lock type. This means that LOCK TABLES on a MERGE table also will implicitly do LOCK TABLES on the children tables. A consequence of this change, is that it is possible to do LOCK TABLES on a child table both explicitly and implicitly with the same statement and that these two locks can be of different strength. For example, LOCK TABLES child READ, merge WRITE. In LOCK TABLES mode, we are not allowed to take new locks and each statement must therefore try to find an existing TABLE instance with a suitable lock. The code that searched for a suitable TABLE instance, only considered table level locks. If a child table was locked twice, it was therefore possible for this code to find a TABLE instance with suitable table level locks but without suitable metadata lock. This problem caused the assert in upgrade_shared_lock_to_exclusive() to be triggered as it tried to upgrade a MDL_SHARED lock to EXCLUSIVE. The problem was a regression caused by the patch for Bug#56292. This patch fixes the problem by partially reverting the changes done by Bug#56292. Now, the children tables will only use the same metadata lock as the MERGE table for MDL_SHARED_NO_WRITE when not in locked tables mode. This means that LOCK TABLE on a MERGE table will not implicitly lock the children tables. This still fixes the original problem in Bug#56292 without causing a regression. Test case added to merge.test. --- mysql-test/r/merge.result | 15 ++++++++++++++- mysql-test/t/merge.test | 24 +++++++++++++++++++++++- storage/myisammrg/ha_myisammrg.cc | 27 +++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/mysql-test/r/merge.result b/mysql-test/r/merge.result index bb6af084f38..ace834a26c2 100644 --- a/mysql-test/r/merge.result +++ b/mysql-test/r/merge.result @@ -3486,12 +3486,13 @@ ALTER TABLE m1 ADD INDEX (c1); UNLOCK TABLES; DROP TABLE m1, t1; # -# Locking the merge table will implicitly lock children. +# Locking the merge table won't implicitly lock children. # CREATE TABLE t1 (c1 INT); CREATE TABLE m1 (c1 INT) ENGINE=MRG_MyISAM UNION=(t1); LOCK TABLE m1 WRITE; ALTER TABLE t1 ADD INDEX (c1); +ERROR HY000: Table 't1' was locked with a READ lock and can't be updated LOCK TABLE m1 WRITE, t1 WRITE; ALTER TABLE t1 ADD INDEX (c1); UNLOCK TABLES; @@ -3661,4 +3662,16 @@ REPAIR TABLE t2 USE_FRM; Table Op Msg_type Msg_text test.t2 repair note The storage engine for the table doesn't support repair DROP TABLE t1, t2; +# +# Bug#57002 Assert in upgrade_shared_lock_to_exclusive() +# for ALTER TABLE + MERGE tables +# +DROP TABLE IF EXISTS t1, m1; +CREATE TABLE t1(a INT) engine=myisam; +CREATE TABLE m1(a INT) engine=merge UNION(t1); +LOCK TABLES t1 READ, m1 WRITE; +ALTER TABLE t1 engine=myisam; +ERROR HY000: Table 't1' was locked with a READ lock and can't be updated +UNLOCK TABLES; +DROP TABLE m1, t1; End of 6.0 tests diff --git a/mysql-test/t/merge.test b/mysql-test/t/merge.test index 34d0b5bf237..07839257a15 100644 --- a/mysql-test/t/merge.test +++ b/mysql-test/t/merge.test @@ -2600,11 +2600,12 @@ UNLOCK TABLES; DROP TABLE m1, t1; --echo # ---echo # Locking the merge table will implicitly lock children. +--echo # Locking the merge table won't implicitly lock children. --echo # CREATE TABLE t1 (c1 INT); CREATE TABLE m1 (c1 INT) ENGINE=MRG_MyISAM UNION=(t1); LOCK TABLE m1 WRITE; +--error ER_TABLE_NOT_LOCKED_FOR_WRITE ALTER TABLE t1 ADD INDEX (c1); LOCK TABLE m1 WRITE, t1 WRITE; ALTER TABLE t1 ADD INDEX (c1); @@ -2776,6 +2777,27 @@ REPAIR TABLE t2 USE_FRM; DROP TABLE t1, t2; +--echo # +--echo # Bug#57002 Assert in upgrade_shared_lock_to_exclusive() +--echo # for ALTER TABLE + MERGE tables +--echo # + +--disable_warnings +DROP TABLE IF EXISTS t1, m1; +--enable_warnings + +CREATE TABLE t1(a INT) engine=myisam; +CREATE TABLE m1(a INT) engine=merge UNION(t1); +LOCK TABLES t1 READ, m1 WRITE; + +# This caused an assert +--error ER_TABLE_NOT_LOCKED_FOR_WRITE +ALTER TABLE t1 engine=myisam; + +UNLOCK TABLES; +DROP TABLE m1, t1; + + --echo End of 6.0 tests --disable_result_log diff --git a/storage/myisammrg/ha_myisammrg.cc b/storage/myisammrg/ha_myisammrg.cc index 466c66ec769..26e54d61101 100644 --- a/storage/myisammrg/ha_myisammrg.cc +++ b/storage/myisammrg/ha_myisammrg.cc @@ -478,8 +478,31 @@ int ha_myisammrg::add_children_list(void) /* Set the expected table version, to not cause spurious re-prepare. */ child_l->set_table_ref_id(mrg_child_def->get_child_table_ref_type(), mrg_child_def->get_child_def_version()); - /* Use the same metadata lock type for children. */ - child_l->mdl_request.set_type(parent_l->mdl_request.type); + /* + For statements which acquire a SNW metadata lock on a parent table and + then later try to upgrade it to an X lock (e.g. ALTER TABLE), SNW + locks should be also taken on the children tables. + + Otherwise we end up in a situation where the thread trying to upgrade SNW + to X lock on the parent also holds a SR metadata lock and a read + thr_lock.c lock on the child. As a result, another thread might be + blocked on the thr_lock.c lock for the child after successfully acquiring + a SR or SW metadata lock on it. If at the same time this second thread + has a shared metadata lock on the parent table or there is some other + thread which has a shared metadata lock on the parent and is waiting for + this second thread, we get a deadlock. This deadlock cannot be properly + detected by the MDL subsystem as part of the waiting happens within + thr_lock.c. By taking SNW locks on the child tables we ensure that any + thread which waits for a thread doing SNW -> X upgrade, does this within + the MDL subsystem and thus potential deadlocks are exposed to the deadlock + detector. + + We don't do the same thing for SNRW locks as this would allow + DDL on implicitly locked underlying tables of a MERGE table. + */ + if (! thd->locked_tables_mode && + parent_l->mdl_request.type == MDL_SHARED_NO_WRITE) + child_l->mdl_request.set_type(MDL_SHARED_NO_WRITE); /* Link TABLE_LIST object into the children list. */ if (this->children_last_l) child_l->prev_global= this->children_last_l;