MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:February 8 2010 8:20pm
Subject:bzr commit into mysql-5.5-next-mr branch (dlenev:3086) Bug#50913
View as plain text  
#At file:///home/dlenev/src/bzr/mysql-next-4284-bg50913/ based on revid:kostja@stripped

 3086 Dmitry Lenev	2010-02-08
      Fix for bug #50913 "Deadlock between open_and_lock_tables_derived
      and MDL".
      
      Concurrent execution of a multi-DELETE statement and ALTER
      TABLE statement which affected one of the tables used in
      the multi-DELETE sometimes led to deadlock.
      Similar deadlocks might have occured when one performed
      INSERT/UPDATE/DELETE on a view and concurrently executed
      ALTER TABLE for the view's underlying table, or when one
      concurrently executed TRUNCATE TABLE for InnoDB table and
      ALTER TABLE for the same table.
      
      These deadlocks were caused by a discrepancy between types of
      metadata and thr_lock.cc locks acquired by those statements.
      
      What happened was that multi-DELETE/TRUNCATE/DML-through-the-
      view statement in the first connection acquired SR lock on a
      table, then ALTER TABLE would come in in the second connection
      and acquire SNW metadata lock and TL_WRITE_ALLOW_READ
      thr_lock.c lock and then would start waiting for the first
      connection during lock upgrade. After that the statement in
      the first connection would try to acquire TL_WRITE lock on
      table and would start waiting for the second connection,
      creating a deadlock.
      
      This patch solves this problem by ensuring that we acquire
      SW metadata lock in all cases in which we acquiring write
      thr_lock.c lock. This guarantees that deadlocks like the
      one described above won't occur since all lock conflicts
      in such situation are resolved within MDL subsystem.
      
      This patch also adds assert which should guarantee that
      such situations won't arise in future.
     @ mysql-test/r/lock_multi.result
        Added main test for bug #50913 "Deadlock between
        open_and_lock_tables_derived and MDL".
     @ mysql-test/r/mdl_sync.result
        Added additional coverage for bug #50913 "Deadlock
        between open_and_lock_tables_derived and MDL".
     @ mysql-test/t/lock_multi.test
        Added main test for bug #50913 "Deadlock between
        open_and_lock_tables_derived and MDL".
     @ mysql-test/t/mdl_sync.test
        Added additional coverage for bug #50913 "Deadlock
        between open_and_lock_tables_derived and MDL".
     @ sql/lock.cc
        Added assert that enforces that when we are locking
        a non-temporary table we have an appropriate type of
        metadata lock on this table.
     @ sql/mysql_priv.h
        Added separate flag for open_tables() to be able specify that
        SH metadata locks on table to be open should be acquired.
        We can no longer use MYSQL_LOCK_IGNORE_FLUSH flag for this
        as in addition to use in I_S implementation it is also used
        for opening system tables. Since in the latter case we also
        acquire thr_lock.c locks using SH metadata lock in it instead
        of SR or SW locks may lead to deadlock.
     @ sql/sql_base.cc
        When opening tables don't interpret MYSQL_LOCK_IGNORE_FLUSH
        flag as request to acquire SH metadata locks. This flag is
        also used for opening system tables for which we also take
        thr_lock.c locks and thus proper metadata lock to take in
        this case is SR or SW lock (otherwise deadlocks can occur).
        In cases when SH lock is really required (e.g. when tables
        are open by I_S implementation) we rely on that newly
        introduced MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL flag is
        used.
     @ sql/sql_delete.cc
        mysql_truncate_by_delete():
          Adjust type of metadata lock to be requested after changing
          type of thr_lock.c lock for table list element from one
          which was set in parser to TL_WRITE.
          This removes discrepancy between types of these locks which
          allowed deadlocks to creep in.
     @ sql/sql_handler.cc
        When closing table which was open by HANDLER statement clear
        TABLE::open_by_handler flag. This allows to use this flag as
        a reliable indication that TABLE instance was open by HANDLER
        statement in assert which was added to mysql_lock_tables().
     @ sql/sql_parse.cc
        multi_delete_set_locks_and_link_aux_tables():
          Adjust type of metadata lock to be requested after changing
          type of thr_lock.c lock for table list element from one
          which was set in parser to TL_WRITE.
          This removes discrepancy between types of these locks which
          allowed deadlocks to creep in.
     @ sql/sql_show.cc
        Use newly introduced MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL
        flag in order to acquire SH metadata locks when opening tables
        in I_S implementation.
     @ sql/sql_update.cc
        Added comment explaining why in multi-update after deciding
        that we need weaker thr_lock.c lock on a table we don't
        downgrade metadata lock on it.
     @ sql/sql_view.cc
        When merging view into main statement adjust type of metadata
        lock to be requested after changing type of thr_lock.c lock
        for table. This removes discrepancy between types of these
        locks which allowed deadlocks to creep in.

    modified:
      mysql-test/r/lock_multi.result
      mysql-test/r/mdl_sync.result
      mysql-test/t/lock_multi.test
      mysql-test/t/mdl_sync.test
      sql/lock.cc
      sql/mysql_priv.h
      sql/sql_base.cc
      sql/sql_delete.cc
      sql/sql_handler.cc
      sql/sql_parse.cc
      sql/sql_show.cc
      sql/sql_update.cc
      sql/sql_view.cc
=== modified file 'mysql-test/r/lock_multi.result'
--- a/mysql-test/r/lock_multi.result	2010-02-01 11:43:06 +0000
+++ b/mysql-test/r/lock_multi.result	2010-02-08 20:19:55 +0000
@@ -302,3 +302,49 @@ LOCK TABLES t1 WRITE;
 FLUSH TABLE t1;
 DROP TABLE t1;
 DROP VIEW v1;
+#
+# Test for bug #50913 "Deadlock between open_and_lock_tables_derived
+# and MDL". Also see additional coverage in mdl_sync.test.
+# 
+drop table if exists t1;
+drop view if exists v1;
+create table t1 (i int);
+create view v1 as select i from t1;
+begin;
+select * from t1;
+i
+# Switching to connection 'con50913'.
+# Sending:
+alter table t1 add column j int;
+# Switching to connection 'default'.
+# Wait until ALTER TABLE gets blocked.
+# The below statement should try to acquire SW lock on 't1'
+# and therefore should get ER_LOCK_DEADLOCK error. Before
+# bug fix it acquired SR lock and hung on thr_lock.c lock.
+delete a from t1 as a where i = 1;
+ERROR 40001: Deadlock found when trying to get lock; try restarting transaction
+# Unblock ALTER TABLE.
+commit;
+# Switching to connection 'con50913'.
+# Reaping ALTER TABLE;
+# Switching to connection 'default'.
+begin;
+select * from v1;
+i
+# Switching to connection 'con50913'.
+# Sending:
+alter table t1 drop column j;
+# Switching to connection 'default'.
+# Wait until ALTER TABLE gets blocked.
+# The below statement should try to acquire SW lock on 't1'
+# and therefore should get ER_LOCK_DEADLOCK error. Before
+# bug fix it acquired SR lock and hung on thr_lock.c lock.
+insert into v1 values (1);
+ERROR 40001: Deadlock found when trying to get lock; try restarting transaction
+# Unblock ALTER TABLE.
+commit;
+# Switching to connection 'con50913'.
+# Reaping ALTER TABLE;
+# Switching to connection 'default'.
+drop view v1;
+drop table t1;

=== modified file 'mysql-test/r/mdl_sync.result'
--- a/mysql-test/r/mdl_sync.result	2010-02-05 14:58:43 +0000
+++ b/mysql-test/r/mdl_sync.result	2010-02-08 20:19:55 +0000
@@ -2292,3 +2292,33 @@ SET DEBUG_SYNC= 'RESET';
 SET @@global.general_log= @old_general_log;
 SET @@global.log_output= @old_log_output;
 SET @@session.sql_log_off= @old_sql_log_off;
+#
+# Additional coverage for bug #50913 "Deadlock between
+# open_and_lock_tables_derived and MDL". The main test
+# case is in lock_multi.test
+#
+drop table if exists t1;
+set debug_sync= 'RESET';
+create table t1 (i int) engine=InnoDB;
+# Switching to connection 'con50913_1'.
+set debug_sync= 'thr_multi_lock_after_thr_lock SIGNAL parked WAIT_FOR go';
+# Sending:
+alter table t1 add column j int;
+# Switching to connection 'default'.
+# Wait until ALTER TABLE gets blocked on a sync point after
+# acquiring thr_lock.c lock.
+set debug_sync= 'now WAIT_FOR parked';
+# The below statement should wait on MDL lock and not deadlock on
+# thr_lock.c lock.
+# Sending:
+truncate table t1;
+# Switching to connection 'con50913_2'.
+# Wait until TRUNCATE TABLE is blocked on MDL lock.
+# Unblock ALTER TABLE.
+set debug_sync= 'now SIGNAL go';
+# Switching to connection 'con50913_1'.
+# Reaping ALTER TABLE.
+# Switching to connection 'default'.
+# Reaping TRUNCATE TABLE.
+set debug_sync= 'RESET';
+drop table t1;

=== modified file 'mysql-test/t/lock_multi.test'
--- a/mysql-test/t/lock_multi.test	2010-02-06 10:28:06 +0000
+++ b/mysql-test/t/lock_multi.test	2010-02-08 20:19:55 +0000
@@ -800,5 +800,82 @@ DROP TABLE t1;
 DROP VIEW v1;
 
 
+--echo #
+--echo # Test for bug #50913 "Deadlock between open_and_lock_tables_derived
+--echo # and MDL". Also see additional coverage in mdl_sync.test.
+--echo # 
+--disable_warnings
+drop table if exists t1;
+drop view if exists v1;
+--enable_warnings
+connect (con50913,localhost,root);
+connection default;
+create table t1 (i int);
+create view v1 as select i from t1;
+begin;
+select * from t1;
+
+--echo # Switching to connection 'con50913'.
+connection con50913;
+--echo # Sending:
+--send alter table t1 add column j int
+
+--echo # Switching to connection 'default'.
+connection default;
+--echo # Wait until ALTER TABLE gets blocked.
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Waiting for table" and info = "alter table t1 add column j int";
+--source include/wait_condition.inc
+--echo # The below statement should try to acquire SW lock on 't1'
+--echo # and therefore should get ER_LOCK_DEADLOCK error. Before
+--echo # bug fix it acquired SR lock and hung on thr_lock.c lock.
+--error ER_LOCK_DEADLOCK
+delete a from t1 as a where i = 1;
+--echo # Unblock ALTER TABLE.
+commit;
+
+--echo # Switching to connection 'con50913'.
+connection con50913;
+--echo # Reaping ALTER TABLE;
+--reap
+
+--echo # Switching to connection 'default'.
+connection default;
+begin;
+select * from v1;
+
+--echo # Switching to connection 'con50913'.
+connection con50913;
+--echo # Sending:
+--send alter table t1 drop column j
+
+--echo # Switching to connection 'default'.
+connection default;
+--echo # Wait until ALTER TABLE gets blocked.
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Waiting for table" and info = "alter table t1 drop column j";
+--source include/wait_condition.inc
+--echo # The below statement should try to acquire SW lock on 't1'
+--echo # and therefore should get ER_LOCK_DEADLOCK error. Before
+--echo # bug fix it acquired SR lock and hung on thr_lock.c lock.
+--error ER_LOCK_DEADLOCK
+insert into v1 values (1);
+--echo # Unblock ALTER TABLE.
+commit;
+
+--echo # Switching to connection 'con50913'.
+connection con50913;
+--echo # Reaping ALTER TABLE;
+--reap
+
+--echo # Switching to connection 'default'.
+connection default;
+disconnect con50913;
+drop view v1;
+drop table t1;
+
+
 # Wait till all disconnects are completed
 --source include/wait_until_count_sessions.inc

=== modified file 'mysql-test/t/mdl_sync.test'
--- a/mysql-test/t/mdl_sync.test	2010-02-03 14:09:27 +0000
+++ b/mysql-test/t/mdl_sync.test	2010-02-08 20:19:55 +0000
@@ -3,6 +3,9 @@
 #
 --source include/have_debug_sync.inc
 
+# We need InnoDB tables for some of the tests.
+--source include/have_innodb.inc
+
 # Save the initial number of concurrent sessions.
 --source include/count_sessions.inc
 
@@ -3315,6 +3318,62 @@ SET @@global.general_log= @old_general_l
 SET @@global.log_output= @old_log_output;
 SET @@session.sql_log_off= @old_sql_log_off;
 
+
+--echo #
+--echo # Additional coverage for bug #50913 "Deadlock between
+--echo # open_and_lock_tables_derived and MDL". The main test
+--echo # case is in lock_multi.test
+--echo #
+--disable_warnings
+drop table if exists t1;
+--enable_warnings
+set debug_sync= 'RESET';
+connect (con50913_1,localhost,root);
+connect (con50913_2,localhost,root);
+connection default;
+create table t1 (i int) engine=InnoDB;
+
+--echo # Switching to connection 'con50913_1'.
+connection con50913_1;
+set debug_sync= 'thr_multi_lock_after_thr_lock SIGNAL parked WAIT_FOR go';
+--echo # Sending:
+--send alter table t1 add column j int
+
+--echo # Switching to connection 'default'.
+connection default;
+--echo # Wait until ALTER TABLE gets blocked on a sync point after
+--echo # acquiring thr_lock.c lock.
+set debug_sync= 'now WAIT_FOR parked';
+--echo # The below statement should wait on MDL lock and not deadlock on
+--echo # thr_lock.c lock.
+--echo # Sending:
+--send truncate table t1
+
+--echo # Switching to connection 'con50913_2'.
+connection con50913_2;
+--echo # Wait until TRUNCATE TABLE is blocked on MDL lock.
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Waiting for table" and info = "truncate table t1";
+--source include/wait_condition.inc
+--echo # Unblock ALTER TABLE.
+set debug_sync= 'now SIGNAL go';
+
+--echo # Switching to connection 'con50913_1'.
+connection con50913_1;
+--echo # Reaping ALTER TABLE.
+--reap
+
+--echo # Switching to connection 'default'.
+connection default;
+--echo # Reaping TRUNCATE TABLE.
+--reap
+disconnect con50913_1;
+disconnect con50913_2;
+set debug_sync= 'RESET';
+drop table t1;
+
+
 # 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 'sql/lock.cc'
--- a/sql/lock.cc	2010-02-04 17:34:15 +0000
+++ b/sql/lock.cc	2010-02-08 20:19:55 +0000
@@ -153,6 +153,25 @@ int mysql_lock_tables_check(THD *thd, TA
     {
       system_count++;
     }
+
+    /*
+      If we are going to lock a non-temporary table we must own metadata
+      lock of appropriate type on it (I.e. for table to be locked for
+      write we must own metadata lock of MDL_SHARED_WRITE or stronger
+      type. For table to be locked for read we must own metadata lock
+      of MDL_SHARED_READ or stronger type).
+      The only exception are HANDLER statements which are allowed to
+      lock table for read while having only MDL_SHARED lock on it.
+    */
+    DBUG_ASSERT(t->s->tmp_table ||
+                thd->mdl_context.is_lock_owner(MDL_key::TABLE,
+                                 t->s->db.str, t->s->table_name.str,
+                                 t->reginfo.lock_type >= TL_WRITE_ALLOW_WRITE ?
+                                 MDL_SHARED_WRITE : MDL_SHARED_READ) ||
+                (t->open_by_handler &&
+                 thd->mdl_context.is_lock_owner(MDL_key::TABLE,
+                                  t->s->db.str, t->s->table_name.str,
+                                  MDL_SHARED)));
   }
 
   /*

=== modified file 'sql/mysql_priv.h'
--- a/sql/mysql_priv.h	2010-02-06 10:28:06 +0000
+++ b/sql/mysql_priv.h	2010-02-08 20:19:55 +0000
@@ -2164,6 +2164,11 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
 #define MYSQL_OPEN_FAIL_ON_MDL_CONFLICT         0x0200
 /** Open tables using MDL_SHARED lock instead of one specified in parser. */
 #define MYSQL_OPEN_FORCE_SHARED_MDL             0x0400
+/**
+  Open tables using MDL_SHARED_HIGH_PRIO lock instead of one specified
+  in parser.
+*/
+#define MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL   0x0800
 
 /** Please refer to the internals manual. */
 #define MYSQL_OPEN_REOPEN  (MYSQL_LOCK_IGNORE_FLUSH |\

=== modified file 'sql/sql_base.cc'
--- a/sql/sql_base.cc	2010-02-06 10:28:06 +0000
+++ b/sql/sql_base.cc	2010-02-08 20:19:55 +0000
@@ -2378,11 +2378,11 @@ open_table_get_mdl_lock(THD *thd, TABLE_
         used in the statement being prepared.
       */
       DBUG_ASSERT(!(flags & (MYSQL_OPEN_TAKE_UPGRADABLE_MDL |
-                             MYSQL_LOCK_IGNORE_FLUSH)));
+                             MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL)));
 
       mdl_request->set_type(MDL_SHARED);
     }
-    else if (flags & MYSQL_LOCK_IGNORE_FLUSH)
+    else if (flags & MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL)
     {
       DBUG_ASSERT(!(flags & MYSQL_OPEN_TAKE_UPGRADABLE_MDL));
       mdl_request->set_type(MDL_SHARED_HIGH_PRIO);

=== modified file 'sql/sql_delete.cc'
--- a/sql/sql_delete.cc	2010-02-06 10:28:06 +0000
+++ b/sql/sql_delete.cc	2010-02-08 20:19:55 +0000
@@ -1052,6 +1052,7 @@ static bool mysql_truncate_by_delete(THD
   bool error, save_binlog_row_based= thd->is_current_stmt_binlog_format_row();
   DBUG_ENTER("mysql_truncate_by_delete");
   table_list->lock_type= TL_WRITE;
+  table_list->mdl_request.set_type(MDL_SHARED_WRITE);
   mysql_init_select(thd->lex);
   thd->clear_current_stmt_binlog_format_row();
   /* Delete all rows from table */

=== modified file 'sql/sql_handler.cc'
--- a/sql/sql_handler.cc	2010-02-05 14:52:17 +0000
+++ b/sql/sql_handler.cc	2010-02-08 20:19:55 +0000
@@ -124,6 +124,7 @@ static void mysql_ha_close_table(THD *th
   {
     /* Non temporary table. */
     tables->table->file->ha_index_or_rnd_end();
+    tables->table->open_by_handler= 0;
     mysql_mutex_lock(&LOCK_open);
     if (close_thread_table(thd, &tables->table))
     {
@@ -332,7 +333,8 @@ bool mysql_ha_open(THD *thd, TABLE_LIST 
               hash_tables->table->s->tmp_table);
   /*
     If it's a temp table, don't reset table->query_id as the table is
-    being used by this handler. Otherwise, no meaning at all.
+    being used by this handler. For non-temp tables we use this flag
+    in asserts.
   */
   hash_tables->table->open_by_handler= 1;
 

=== modified file 'sql/sql_parse.cc'
--- a/sql/sql_parse.cc	2010-02-06 10:28:06 +0000
+++ b/sql/sql_parse.cc	2010-02-08 20:19:55 +0000
@@ -7061,6 +7061,9 @@ bool multi_delete_set_locks_and_link_aux
     }
     walk->updating= target_tbl->updating;
     walk->lock_type= target_tbl->lock_type;
+    /* We can assume that tables to be deleted from are locked for write. */
+    DBUG_ASSERT(walk->lock_type >= TL_WRITE_ALLOW_WRITE);
+    walk->mdl_request.set_type(MDL_SHARED_WRITE);
     target_tbl->correspondent_table= walk;	// Remember corresponding table
   }
   DBUG_RETURN(FALSE);

=== modified file 'sql/sql_show.cc'
--- a/sql/sql_show.cc	2010-02-06 10:28:06 +0000
+++ b/sql/sql_show.cc	2010-02-08 20:19:55 +0000
@@ -2928,6 +2928,7 @@ fill_schema_show_cols_or_idxs(THD *thd, 
   lex->sql_command= SQLCOM_SHOW_FIELDS;
   res= open_normal_and_derived_tables(thd, show_table_list,
                                       (MYSQL_LOCK_IGNORE_FLUSH |
+                                       MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL |
                                        (can_deadlock ?
                                         MYSQL_OPEN_FAIL_ON_MDL_CONFLICT : 0)));
   lex->sql_command= save_sql_command;
@@ -3507,6 +3508,7 @@ int get_all_tables(THD *thd, TABLE_LIST 
               schema_table->i_s_requested_object;
             res= open_normal_and_derived_tables(thd, show_table_list,
                    (MYSQL_LOCK_IGNORE_FLUSH |
+                    MYSQL_OPEN_FORCE_SHARED_HIGH_PRIO_MDL |
                     (can_deadlock ? MYSQL_OPEN_FAIL_ON_MDL_CONFLICT : 0)));
             lex->sql_command= save_sql_command;
             /*

=== modified file 'sql/sql_update.cc'
--- a/sql/sql_update.cc	2010-02-06 10:28:06 +0000
+++ b/sql/sql_update.cc	2010-02-08 20:19:55 +0000
@@ -1051,6 +1051,11 @@ reopen_tables:
         If we are using the binary log, we need TL_READ_NO_INSERT to get
         correct order of statements. Otherwise, we use a TL_READ lock to
         improve performance.
+        We don't downgrade metadata lock from SW to SR in this case as
+        there is no guarantee that the same ticket is not used by
+        another table instance used by this statement which is going to
+        be write-locked (for example, trigger to be invoked might try
+        to update this table).
       */
       tl->lock_type= read_lock_type_for_table(thd, table);
       tl->updating= 0;

=== modified file 'sql/sql_view.cc'
--- a/sql/sql_view.cc	2010-02-06 10:28:06 +0000
+++ b/sql/sql_view.cc	2010-02-08 20:19:55 +0000
@@ -1347,7 +1347,11 @@ bool mysql_make_view(THD *thd, File_pars
         anyway.
       */
       for (tbl= view_main_select_tables; tbl; tbl= tbl->next_local)
+      {
         tbl->lock_type= table->lock_type;
+        tbl->mdl_request.set_type((tbl->lock_type >= TL_WRITE_ALLOW_WRITE) ?
+                                  MDL_SHARED_WRITE : MDL_SHARED_READ);
+      }
       /*
         If the view is mergeable, we might want to
         INSERT/UPDATE/DELETE into tables of this view. Preserve the


Attachment: [text/bzr-bundle] bzr/dlenev@mysql.com-20100208201955-ma7wws6qhymcovox.bundle
Thread
bzr commit into mysql-5.5-next-mr branch (dlenev:3086) Bug#50913Dmitry Lenev8 Feb
  • Re: bzr commit into mysql-5.5-next-mr branch (dlenev:3086) Bug#50913Konstantin Osipov9 Feb