List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:August 20 2010 4:35pm
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (mattias.jonsson:3184)
Bug#54747
View as plain text  
Hello Mattias!

* Mattias Jonsson <mattias.jonsson@stripped> [10/08/20 19:32]:
> #At file:///Users/mattiasj/mysql-bzr/b54747-5.5-bf/ based on
> revid:joerg@stripped
> 
>  3184 Mattias Jonsson	2010-08-20
>       Bug#54747: Deadlock between REORGANIZE PARTITION and SELECT is not detected
>       
>       The ALTER PARTITION and SELECT seemed to be deadlocked
>       when having innodb_thread_concurrency = 1.
>       
>       Problem was that there was unreleased latches
>       in the ALTER PARTITION thread which was needed
>       by the SELECT thread to be able to continue.
>       
>       Solution was to release the latches by commit 
>       before requesting upgrade to exclusive MDL lock.

IMO it makes sense to be a bit more elaborate and explicit in ChangeSet
comment. For example I would have written something like this:

"
Concurrent execution of ALTER ... PARTITION statement and transaction
or statement which accessed to the InnoDB table being ALTERed could
have led to deadlock when @@innodb_thread_concurrency system variable
was set to 1. The same problem might have occurred with a bigger value
of @@innodb_thread_concurrency but was less probable.

This deadlock occured because ALTER ... PARTITION was not releasing
InnoDB concurrency ticket before starting to wait for metadata lock
upgrade. If connection that owned metadata lock blocking this upgrade
tried to acquire InnoDB concurrency ticket (by accessing any InnoDB
table) it has to wait for ALTER and thus deadlock occurred.

This patch solves this problem by releasing InnoDB latches, which
include concurrency ticket, by commiting transaction before ALTER ...
PARTITION requests upgrade to exclusive metadata lock.
"

>       
>       Updated according to reviewers comments (2).
>      @ mysql-test/r/partition_innodb.result
>         updated test result
>      @ mysql-test/t/partition_innodb.test
>         added test
>      @ sql/sql_partition.cc
>         Moved implicit commit into mysql_change_partition
>         so that if latches are taken, they are always released
>         before waiting on exclusive lock.
>      @ sql/sql_table.cc
>         refactored the code to prepare and commit
>         around copy_data_between_tables, to be able
>         to reuse it in mysql_change_partitions
>      @ sql/sql_table.h
>         exporting mysql_trans_prepare/commit_alter_copy_data
> 

...

> === modified file 'mysql-test/t/partition_innodb.test'
> --- a/mysql-test/t/partition_innodb.test	2010-08-13 07:50:25 +0000
> +++ b/mysql-test/t/partition_innodb.test	2010-08-20 15:24:18 +0000
> @@ -9,6 +9,61 @@ drop table if exists t1, t2;
>  let $MYSQLD_DATADIR= `SELECT @@datadir`;
>  
>  --echo #
> +--echo # Bug#54747: Deadlock between REORGANIZE PARTITION and
> +--echo #            SELECT is not detected
> +--echo #
> +
> +SET @old_innodb_thread_concurrency:= @@innodb_thread_concurrency;
> +SET GLOBAL innodb_thread_concurrency = 1;
> +
> +CREATE TABLE t1
> +(user_num BIGINT,
> + hours SMALLINT,
> + KEY user_num (user_num))
> +ENGINE = InnoDB   
> +PARTITION BY RANGE COLUMNS (hours)
> +(PARTITION hour_003 VALUES LESS THAN (3),
> + PARTITION hour_004 VALUES LESS THAN (4),
> + PARTITION hour_005 VALUES LESS THAN (5),
> + PARTITION hour_last VALUES LESS THAN (MAXVALUE));
> +
> +INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
> +
> +BEGIN;
> +SELECT COUNT(*) FROM t1;
> +
> +--echo # Start a ALTER PARTITION and wait between the copy and rename of table.
> +--echo # con1
> +
> +--connect (con1,localhost,root,,)

Please consider adding something like:
--echo # Connection 'con1'
before switching connection here and _in other places_ in your
test case. This makes test result easier to understand and makes
test easier to maintain.
> +--send
> +ALTER TABLE t1
> +REORGANIZE PARTITION hour_003, hour_004 INTO
> +(PARTITION oldest VALUES LESS THAN (4));

Please consider adding something like:
--echo # Sending:
before above operator. Again this makes test case result easier
to understand and makes easier to debug test in case of failure.

> +
> +
> +--connection default
> +let $wait_condition =
> +SELECT COUNT(*) = 1
> +FROM information_schema.processlist
> +WHERE INFO like 'ALTER TABLE t1%REORGANIZE PARTITION hour_003, hour_004%'
> +AND STATE = 'Waiting for table';
> +--source include/wait_condition.inc

Please consider echoing comment before this wait explaining
what are you waiting for.

BTW after pull from mysql-5.5-bugfixing the thread state name will
change from "Waiting for table" to "Waiting for table metadata lock".

> +SELECT COUNT(*) FROM t1;
> +COMMIT;
> +
> +--connection con1
> +--reap

Please consider adding something like:
--echo # Reaping ALTER TABLE.
before this command to make test result more readable and the
whole test easier to maintain and debug.

> +
> +--disconnect con1
> +
> +--connection default
> +
> +SET GLOBAL innodb_thread_concurrency = @old_innodb_thread_concurrency;
> +DROP TABLE t1;
> +
> +
> +--echo #
>  --echo # Bug#50418: DROP PARTITION does not interact with transactions
>  --echo #
>  CREATE TABLE t1 (
> 

...

I think it is OK to push this patch after considering the above
suggestions.

-- 
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-bugfixing branch (mattias.jonsson:3184) Bug#54747Mattias Jonsson20 Aug
  • Re: bzr commit into mysql-5.5-bugfixing branch (mattias.jonsson:3184)Bug#54747Dmitry Lenev20 Aug