List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 8 2010 7:05am
Subject:Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3124)
Bug#56292
View as plain text  
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
Thread
bzr commit into mysql-5.5-runtime branch (jon.hauglid:3124) Bug#56292Jon Olav Hauglid30 Aug
  • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3124)Bug#56292Dmitry Lenev8 Sep