MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 9 2009 8:33am
Subject:Re: bzr commit into mysql-5.4 branch (jon.hauglid:2877) Bug#42147
View as plain text  
Hello Jon Olav!

Here are some comments about your patch:

* Jon Olav Hauglid <Jon.Hauglid@stripped> [09/08/25 12:42]:
> #At file:///export/home/z/mysql-next-bugfixing-bug42147/ based on
> revid:jon.hauglid@stripped
> 
>  2877 Jon Olav Hauglid	2009-08-25
>       Bug #42147 Concurrent DML and LOCK TABLE ... READ for InnoDB 
>                  table cause warnings in errlog
>             

I think that even for ChangeSet that does not fix the problem it makes
sense to explain what the problem is and what is its cause. This will
provide proper context for your further explanations. Also it will give
you opportunity to explain why this problem with locking doesn't lead
to any severe consequences and therefore can be ignored at the moment.

These explanations can look like:
--
Concurrent execution of LOCK TABLES ... READ statement for InnoDB table
and DML statements affecting the same table on the debug build of the
MySQL server might lead to "Found lock of type 6 that is write and read
locked" warnings appearing in error log.

The problem is that table-level locking code allows thread to acquire
TL_READ_NO_INSERT lock on the table even if there is another thread
which holds TL_WRITE_ALLOW_WRITE lock on this table while assuming 
at the same time that such locks are incompatible (for example, see
check_locks() code).

This doesn't lead to any problems other than warnings in error log for
debug build of server since for InnoDB tables TL_READ_NO_INSERT type of
lock is only used for LOCK TABLES and for this statement InnoDB also
performs its own table-level locking.
--

>       The table lock compatibility matrix cannnot be updated to allow
                                            ^^^^^^^               ^^^^^^^^
                                           "cannot" :)            disallow

>       TL_READ_NO_INSERT when another connection holds TL_WRITE_ALLOW_WRITE
>       without causing starvation of LOCK TABLE READ in InnoDB under
>       high write load. This patch therefore contains no code changes.
>       
>       The issue will be fixed later when LOCK TABLE READ has been updated
>       to not use table locks. This bug will therefore be marked as 
>       "To be fixed later".
>       
>       Code comment in thr_lock.c expanded to clarify the issue and a 
>       test case based on the bug description added to innodb_mysql_lock.test.
>       Note that this test case for now suppresses the warnings generated by InnoDB.

Actually, the warnings which this patch suppresses are generated by
thr_lock.c and not by InnoDB engine. Please update the above comment
accordingly.

...

> === modified file 'mysql-test/t/innodb_mysql_lock.test'
> --- a/mysql-test/t/innodb_mysql_lock.test	2009-07-08 12:09:38 +0000
> +++ b/mysql-test/t/innodb_mysql_lock.test	2009-08-25 08:41:34 +0000
> @@ -1,4 +1,5 @@
>  -- source include/have_innodb.inc
> +-- source include/have_debug_sync.inc
> 

May be we can avoid using DEBUG_SYNC facility for this test?
So test for this bug and other tests in this file will be
able to run for non-debug builds of the server as well.
See more below...

>  --echo #
>  --echo # Bug #22876 Four-way deadlock
> @@ -51,8 +52,50 @@ INSERT INTO t1 VALUES (2);
>  connection con2;
>  --reap
>  commit;
> +set @@autocommit=1;
>  connection con1;
>  commit;
> +set @@autocommit=1;
>  connection con3;
>  --reap
> +set @@autocommit=1;
>  connection default;
> +
> +disconnect con1;
> +disconnect con3;
> +
> +--echo #
> +--echo # Bug #42147 Concurrent DML and LOCK TABLE ... READ for InnoDB 
> +--echo #            table cause warnings in errlog
> +--echo #
> +
> +--echo # Adding suppressions for warnings generated by this test
> +call mtr.add_suppression("Found lock of type 6 that is write and read locked");

IMO it is better to add global suppression instead.
This way it will also cover other test cases which might be affected
by this bug (for example there are such test cases in mysql-fk tree).
Also this way your test case will make more sense as it will also test
for the presence of such global suppression.

> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +--enable_warnings
> +
> +CREATE TABLE t1 (i INT) engine= innodb;
> +
> +--echo # Connection 1
> +connection default;
> +SET DEBUG_SYNC='after_lock_tables_takes_lock SIGNAL tbl_locked WAIT_FOR
> tbl_unlock';
> +--send INSERT INTO t1 VALUES (0)
> +

I think, instead of using "after_lock_tables_takes_lock" sync point
to block INSERT execution after it acquires TL_WRITE_ALLOW_WRITE lock
it is better to use user-level locks as this will allow to run this
test on non-debug builds of server. For examples of usage of user-level
locks see test for bug #26141 in trigger-trans.test and various tests
in events_bugs.test.

> +--echo # Connection 2
> +connection con2;
> +SET DEBUG_SYNC='now WAIT_FOR tbl_locked';
> +SET DEBUG_SYNC='before_lock_tables_takes_lock SIGNAL tbl_unlock';
> +--send LOCK TABLES t1 READ
> +
> +connection default;
> +--reap
> +
> +connection con2;
> +--reap
> +UNLOCK TABLES;
> +
> +connection default;
> +SET DEBUG_SYNC='RESET';
> +DROP TABLE t1;

Please don't forget to disconnect 'con2' as part of clean-up.

Also I think you should add include/wait_until_count_sessions.inc
to the end of this test file in order to check that all connections
opened in it are really gone so execution of other tests won't be
affected by their presence. See mdl_sync.test for example of usage
of this script.

> 
> === modified file 'mysys/thr_lock.c'
> --- a/mysys/thr_lock.c	2009-03-11 17:17:00 +0000
> +++ b/mysys/thr_lock.c	2009-08-25 08:41:34 +0000
> @@ -564,13 +564,31 @@ thr_lock(THR_LOCK_DATA *data, THR_LOCK_O
>      if (lock->write.data)
>      {
>        /*
> -        We can allow a read lock even if there is already a write lock
> -	 on the table in one the following cases:
> -	 - This thread alread have a write lock on the table
> -	 - The write lock is TL_WRITE_ALLOW_READ or TL_WRITE_DELAYED
> -           and the read lock is TL_READ_HIGH_PRIORITY or TL_READ
> -         - The write lock is TL_WRITE_CONCURRENT_INSERT or TL_WRITE_ALLOW_WRITE
> -	   and the read lock is not TL_READ_NO_INSERT
> +        We can allow a read lock even if there is already a
> +        write lock on the table if they are owned by the same
> +        thread or if they satisfy the following lock
> +        compatibility matrix:
> +
> +           Request
> +          /-------
> +         H|+++++  WRITE_ALLOW_WRITE
> +         e|++++-  WRITE_ALLOW_READ
> +         l|++++-  WRITE_CONCURRENT_INSERT
> +         d|+++++  WRITE_DELAYED
> +           |||||
> +           ||||\= READ_NO_INSERT
> +           |||\ = READ_HIGH_PRIORITY
> +           ||\  = READ_WITH_SHARED_LOCKS
> +           |\   = READ
> +           \    = READ_DEFAULT

TL_READ_DEFAULT is not a "real" lock type but a special lock type which
is set for tables at parsing stage and which at open_tables() becomes
either TL_READ or TL_READ_NO_INSERT depending on the binary log format
and on the table category. So it should not be present in this matrix.

> +
> +        + = Request can be satisified.
> +        - = Request cannot be satisified.
> +
> +        READ_NO_INSERT and WRITE_ALLOW_WRITE should in principle
> +        be incompatible. However this will cause starcation of
                                                    ^^^^^^^^^^
                                                   "starvation" :)

> +        LOCK TABLE READ in InnoDB under high write load.
> +        See Bug#42147 for more information.
>        */
>  
>        DBUG_PRINT("lock",("write locked 1 by thread: 0x%lx",
> 

-- 
Dmitry Lenev, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.4 branch (jon.hauglid:2877) Bug#42147Jon Olav Hauglid25 Aug
  • Re: bzr commit into mysql-5.4 branch (jon.hauglid:2877) Bug#42147Dmitry Lenev9 Sep