Hello Jon Olav!
* Jon Olav Hauglid <jon.hauglid@stripped> [10/08/30 17:46]:
> #At file:///export/home/x/mysql-5.5-runtime-bug56292/ based on
> revid:alik@stripped
>
> 3124 Jon Olav Hauglid 2010-08-30
> Bug #56292 Deadlock with ALTER TABLE and MERGE tables
>
> ALTER TABLE on a MERGE table could cause a deadlock with two
> other connections if we reached a situation where:
>
> 1) A connection doing ALTER TABLE can't upgrade to MDL_EXCLUSIVE on the
> parent table, but holds TL_READ_NO_INSERT on the child tables.
> 2) A connection doing DELETE on a child table can't get TL_WRITE on it
> since ALTER TABLE holds TL_READ_NO_INSERT.
> 3) A connection doing SELECT on the parent table can't get TL_READ on
> the child tables since TL_WRITE is ahead in the lock queue, but holds
> MDL_SHARED_READ on the parent table preventing ALTER TABLE from upgrading.
>
> For regular tables, this deadlock is avoided by having ALTER TABLE
> take a MDL_SHARED_NO_WRITE metadata lock on the table. This prevents
> DELETE from acquiring MDL_SHARED_WRITE on the table before ALTER TABLE
> tries to upgrade to MDL_EXCLUSIVE. In the example above, SELECT would
> therefore not be blocked by the pending DELETE as DELETE would not be
> able to enter TL_WRITE in the table lock queue.
>
> This patch fixes the problem for merge tables by using the same metadata
> lock type for child tables as for the parent table. The child tables will
> in this case therefore be locked with MDL_SHARED_NO_WRITE, preventing
> DELETE from acquiring a metadata lock and enter into the table lock queue.
>
> Change in behavior: By taking the same metadata lock for child tables
> as for the parent table, LOCK TABLE on the parent table will now also
> implicitly lock the child tables. Merge.test/.result has been updated
> to reflect this change.
I think it is also worth to mention that after this patch LOCK TABLE ...
WRITE for MERGE tables or underlying tables of MERGE tables might end up
with ER_LOCK_DEADLOCK error.
...
> === modified file 'mysql-test/t/mdl_sync.test'
> --- a/mysql-test/t/mdl_sync.test 2010-08-12 13:50:23 +0000
> +++ b/mysql-test/t/mdl_sync.test 2010-08-30 10:58:29 +0000
> @@ -4532,6 +4532,68 @@ disconnect con2;
> disconnect con3;
>
>
> +--echo #
> +--echo # Bug#56292 Deadlock with ALTER TABLE and MERGE tables
> +--echo #
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1, t2, m1;
> +--enable_warnings
> +
> +CREATE TABLE t1(a INT) engine=MyISAM;
> +CREATE TABLE t2(a INT) engine=MyISAM;
> +CREATE TABLE m1(a INT) engine=MERGE UNION=(t1, t2);
> +
> +INSERT INTO t1 VALUES (1), (2);
> +INSERT INTO t2 VALUES (3), (4);
> +
> +connect(con1, localhost, root);
> +connect(con2, localhost, root);
> +
> +--echo # Connection con1
> +connection con1;
> +SET DEBUG_SYNC= 'mdl_upgrade_shared_lock_to_exclusive SIGNAL upgrade WAIT_FOR
> continue';
> +--echo # Sending:
> +--send ALTER TABLE m1 engine=MERGE UNION=(t2, t1)
> +
> +--echo # Connection con2
> +connection con2;
> +--echo # Waiting for ALTER TABLE to try lock upgrade
> +SET DEBUG_SYNC= 'now WAIT_FOR upgrade';
> +--echo # Sending:
> +--send DELETE FROM t2 WHERE a = 3
> +
> +--echo # Connection default
> +connection default;
> +--echo # Check that DELETE is waiting on a metadata lock and not a table lock.
> +let $wait_condition=
> + SELECT COUNT(*) = 1 FROM information_schema.processlist
> + WHERE state = "Waiting for table metadata lock" AND
> + info = "DELETE FROM t2 WHERE a = 3";
> +--source include/wait_condition.inc
> +# Now that DELETE blocks on a metadata lock, we should be able to do
> +# SELECT * FROM m1 here. SELECT used to be blocked by a DELETE table
> +# lock request.
Maybe it makes sense to add --echo for this comment as well ?
> +SELECT * FROM m1;
> +--echo # Resuming ALTER TABLE
> +SET DEBUG_SYNC= 'now SIGNAL continue';
> +
> +--echo # Connection con1
> +connection con1;
> +--echo # Reaping: ALTER TABLE m1 engine=MERGE UNION=(t2, t1)
> +--reap
> +--echo # Connection con2
> +connection con2;
> +--echo # Reaping: DELETE FROM t2 WHERE a = 3
> +--reap
> +--echo # Connection default
> +connection default;
> +DROP TABLE m1, t1, t2;
> +SET DEBUG_SYNC= 'RESET';
> +disconnect con1;
> +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
>
...
> === modified file 'storage/myisammrg/ha_myisammrg.cc'
> --- a/storage/myisammrg/ha_myisammrg.cc 2010-07-27 14:32:42 +0000
> +++ b/storage/myisammrg/ha_myisammrg.cc 2010-08-30 10:58:29 +0000
> @@ -478,6 +478,8 @@ 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 MDL lock type for children. */
> + child_l->mdl_request.set_type(parent_l->mdl_request.type);
I think it is better not to use abbreviation MDL (metadata lock)
and word lock together. How about rephrasing this comment to:
/* Use the same metadata lock type for children. */
?
> /* Link TABLE_LIST object into the children list. */
> if (this->children_last_l)
> child_l->prev_global= this->children_last_l;
>
I think it is OK to push this patch after considering above points.
--
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
