Fix for bug #51263 "Deadlock between transactional

SELECT and ALTER TABLE ...  REBUILD PARTITION".

ALTER TABLE on InnoDB table (including partitioned tables)
acquired exclusive locks on rows of table being altered.
In cases when there was concurrent transaction which did
locking reads from this table this sometimes led to a
deadlock which was not detected by MDL subsystem nor by
InnoDB engine (and was reported only after exceeding
innodb_lock_wait_timeout).

This problem stemmed from the fact that ALTER TABLE acquired
TL_WRITE_ALLOW_READ lock on table being altered. This lock
was interpreted as a write lock and thus for table being
altered handler::external_lock() method was called with
F_WRLCK as an argument. As result InnoDB engine treated
ALTER TABLE as an operation which is going to change data
and acquired LOCK_X locks on rows being read from old
version of table.

In case when there was a transaction which already acquired
SR metadata lock on table and some LOCK_S locks on its rows
(e.g. by using it in subquery of DML statement) concurrent
ALTER TABLE was blocked at the moment when it tried to
acquire LOCK_X lock before reading one of these rows.
The transaction's attempt to acquire SW metadata lock on
table being altered led to deadlock, since it had to wait
for ALTER TABLE to release SNW lock. This deadlock was not
detected and got resolved only after timeout expiring
because waiting were happening in two different subsystems.

Similar deadlocks could have occured in other situations.
This patch tries to solve the problem by changing ALTER TABLE
implementation to use TL_READ_NO_INSERT lock instead of
TL_WRITE_ALLOW_READ. After this step handler::external_lock()
is called with F_RDLCK as an argument and InnoDB engine
correctly interprets ALTER TABLE as operation which only
reads data from original version of table. Thanks to this
ALTER TABLE acquires only LOCK_S locks on rows it reads.
This, in its turn, causes inter-subsystem deadlocks to go
away, as all potential lock conflicts and thus deadlocks will
be limited to metadata locking subsystem:

- When ALTER TABLE reads rows from table being altered it
  can't encounter any locks which conflict with LOCK_S row
  locks. There should be no concurrent transactions holding
  LOCK_X row locks. Such a transaction should have been
  acquired SW metadata lock on table first which would have
  conflicted with ALTER's SNW lock.
- Vice versa, when DML which runs concurrently with ALTER
  TABLE tries to lock row it should be requesting only LOCK_S
  lock which is compatible with locks acquired by ALTER,
  as otherwise such DML must own an SW metadata lock on table
  which would be incompatible with ALTER's SNW lock.

mysql-test/r/innodb_mysql_lock2.result:
  Added test for bug #51263 "Deadlock between transactional
  SELECT and ALTER TABLE ... REBUILD PARTITION".
mysql-test/suite/rpl_ndb/r/rpl_ndb_binlog_format_errors.result:
  Since CREATE TRIGGER no longer acquires write lock on table
  it is no longer interpreted as an operation which modifies
  table data and therefore no longer fails if invoked for
  SBR-only engine in ROW mode.
mysql-test/suite/rpl_ndb/t/rpl_ndb_binlog_format_errors.test:
  Since CREATE TRIGGER no longer acquires write lock on table
  it is no longer interpreted as an operation which modifies
  table data and therefore no longer fails if invoked for
  SBR-only engine in ROW mode.
mysql-test/t/innodb_mysql_lock2.test:
  Added test for bug #51263 "Deadlock between transactional
  SELECT and ALTER TABLE ... REBUILD PARTITION".
sql/ha_partition.cc:
  When ALTER TABLE creates a new partition to be filled from
  other partition lock it in F_WRLCK mode instead of using
  mode which was used for locking the whole table (it is
  F_RDLCK now).
sql/lock.cc:
  Replaced conditions which used TL_WRITE_ALLOW_READ
  lock type with equivalent conditions using
  TL_WRITE_ALLOW_WRITE. This should allow to get rid
  of TL_WRITE_ALLOW_READ lock type eventually.
sql/mdl.cc:
  Updated outdated comment to reflect current situation.
sql/sql_base.cc:
  Replaced conditions which used TL_WRITE_ALLOW_READ
  lock type with equivalent conditions using
  TL_WRITE_ALLOW_WRITE. This should allow to get rid
  of TL_WRITE_ALLOW_READ lock type eventually.
sql/sql_table.cc:
  mysql_admin_table():
    Use TL_WRITE_ALLOW_WRITE lock type instead of
    TL_WRITE_ALLOW_READ to determine that we need to acquire
    upgradable metadata lock. This should allow to completely
    get rid of TL_WRITE_ALLOW_READ in long term.
  mysql_recreate_table():
    ALTER TABLE now requires TL_READ_NO_INSERT thr_lock.c lock
    instead of TL_WRITE_ALLOW_READ.
sql/sql_trigger.cc:
  Changed CREATE/DROP TRIGGER implementation to use
  TL_READ_NO_INSERT lock instead of TL_WRITE_ALLOW_READ lock.
  The latter is no longer necessary since:
  a) We now can rely on metadata locks to achieve proper
     isolation between two DDL statements or DDL and DML
     statements.
  b) This statement does not change any data in table so there
     is no need to inform storage engine about it.
sql/sql_yacc.yy:
  Changed implementation of ALTER TABLE (and CREATE/DROP INDEX
  as a consequence) to use TL_READ_NO_INSERT lock instead of
  TL_WRITE_ALLOW_READ lock. This is possible since:
  a) We now can rely on metadata locks to achieve proper
     isolation between two DDL statements or DDL and DML
     statements.
  b) This statement only reads data in table being open.
     We write data only to the new version of table and
     then replace with it old version of table under
     X metadata lock.
  
  Thanks to this change InnoDB will no longer acquire LOCK_X
  locks on rows being read by ALTER TABLE (instead LOCK_S
  locks will be acquired) and thus cause of bug #51263
  "Deadlock between transactional SELECT and ALTER TABLE ...
  REBUILD PARTITION" is removed.
  
  Did the similar change for CREATE TRIGGER (see comments
  for sql_trigger.cc for details).

11 files changed, 186 insertions, 19 deletions

diff --git a/mysql-test/r/innodb_mysql_lock2.result b/mysql-test/r/innodb_mysql_lock2.result
index aed704e6b3e..70cfbee6463 100644
--- a/mysql-test/r/innodb_mysql_lock2.result
+++ b/mysql-test/r/innodb_mysql_lock2.result
@@ -562,3 +562,68 @@ drop view v1, v2;
 drop procedure p1;
 drop procedure p2;
 drop table t1, t2, t3, t4, t5;
+#
+# Test for bug#51263 "Deadlock between transactional SELECT
+# and ALTER TABLE ... REBUILD PARTITION".
+#
+drop table if exists t1, t2;
+create table t1 (i int auto_increment not null primary key) engine=innodb;
+create table t2 (i int) engine=innodb;
+insert into t1 values (1), (2), (3), (4), (5);
+begin;
+# Acquire SR metadata lock on t1 and LOCK_S row-locks on its rows.
+insert into t2 select count(*) from t1;
+# Switching to connection 'con1'.
+# Sending:
+alter table t1 add column j int;
+# Switching to connection 'default'.
+# Wait until ALTER is blocked because it tries to upgrade SNW
+# metadata lock to X lock.
+# It should not be blocked during copying data to new version of
+# table as it acquires LOCK_S locks on rows of old version, which
+# are compatible with locks acquired by connection 'con1'.
+# The below statement will deadlock because it will try to acquire
+# SW lock on t1, which will conflict with ALTER's SNW lock. And
+# ALTER will be waiting for this connection to release its SR lock.
+# This deadlock should be detected by an MDL subsystem and this
+# statement should be aborted with an appropriate error.
+insert into t1 values (6);
+ERROR 40001: Deadlock found when trying to get lock; try restarting transaction
+# Unblock ALTER TABLE.
+commit;
+# Switching to connection 'con1'.
+# Reaping ALTER TABLE.
+# Switching to connection 'default'.
+#
+# Now test for scenario in which bug was reported originally.
+#
+drop tables t1, t2;
+create table t1 (i int auto_increment not null primary key) engine=innodb
+partition by hash (i) partitions 4;
+create table t2 (i int) engine=innodb;
+insert into t1 values (1), (2), (3), (4), (5);
+begin;
+# Acquire SR metadata lock on t1.
+select * from t1;
+i
+1
+2
+3
+4
+5
+# Switching to connection 'con1'.
+# Sending:
+alter table t1 rebuild partition p0;
+# Switching to connection 'default'.
+# Wait until ALTER is blocked because of active SR lock.
+# The below statement should succeed as transaction
+# has SR metadata lock on t1 and only going to read
+# rows from it.
+insert into t2 select count(*) from t1;
+# Unblock ALTER TABLE.
+commit;
+# Switching to connection 'con1'.
+# Reaping ALTER TABLE.
+# Switching to connection 'default'.
+# Clean-up.
+drop tables t1, t2;
diff --git a/mysql-test/suite/rpl_ndb/r/rpl_ndb_binlog_format_errors.result b/mysql-test/suite/rpl_ndb/r/rpl_ndb_binlog_format_errors.result
index 36b8e022dd2..ab126026fe4 100644
--- a/mysql-test/suite/rpl_ndb/r/rpl_ndb_binlog_format_errors.result
+++ b/mysql-test/suite/rpl_ndb/r/rpl_ndb_binlog_format_errors.result
@@ -43,9 +43,9 @@ SELECT * FROM t /* Should be empty */;
 a
 * Modify both row-only and stmt-only table
 CREATE TRIGGER trig_2 AFTER INSERT ON t_stmt FOR EACH ROW BEGIN INSERT INTO t_row VALUES(1); END;
-ERROR HY000: Cannot execute statement: impossible to write to binary log since BINLOG_FORMAT = ROW and at least one table uses a storage engine limited to statement-based logging.
 INSERT INTO t_stmt VALUES (1);
-ERROR HY000: Cannot execute statement: impossible to write to binary log since BINLOG_FORMAT = ROW and at least one table uses a storage engine limited to statement-based logging.
+ERROR HY000: Cannot execute statement: impossible to write to binary log since both row-incapable engines and statement-incapable engines are involved.
+DROP trigger trig_2;
 SELECT * FROM t_stmt /* should be empty */;
 a
 * Stmt-only table and binlog_format=row
diff --git a/mysql-test/suite/rpl_ndb/t/rpl_ndb_binlog_format_errors.test b/mysql-test/suite/rpl_ndb/t/rpl_ndb_binlog_format_errors.test
index 481db5f6564..f6331cd0808 100644
--- a/mysql-test/suite/rpl_ndb/t/rpl_ndb_binlog_format_errors.test
+++ b/mysql-test/suite/rpl_ndb/t/rpl_ndb_binlog_format_errors.test
@@ -97,10 +97,10 @@ SELECT * FROM t_self_logging /* Should be empty */;
 SELECT * FROM t /* Should be empty */;
 
 --echo * Modify both row-only and stmt-only table
---error ER_BINLOG_ROW_MODE_AND_STMT_ENGINE
 --eval CREATE TRIGGER trig_2 AFTER INSERT ON t_stmt FOR EACH ROW BEGIN INSERT INTO t_row VALUES(1); END
---error ER_BINLOG_ROW_MODE_AND_STMT_ENGINE
+--error ER_BINLOG_ROW_ENGINE_AND_STMT_ENGINE
 INSERT INTO t_stmt VALUES (1);
+DROP trigger trig_2;
 SELECT * FROM t_stmt /* should be empty */;
 
 --echo * Stmt-only table and binlog_format=row
diff --git a/mysql-test/t/innodb_mysql_lock2.test b/mysql-test/t/innodb_mysql_lock2.test
index 5111d56225a..1f01f4cb010 100644
--- a/mysql-test/t/innodb_mysql_lock2.test
+++ b/mysql-test/t/innodb_mysql_lock2.test
@@ -3,7 +3,12 @@
 # This test requires statement/mixed mode binary logging.
 # Row-based mode puts weaker serializability requirements
 # so weaker locks are acquired for it.
+# Also in ROW mode LOCK_S row locks won't be acquired for DML
+# and test for bug#51263 won't trigger execution path on which
+# this bug was encountered.
 --source include/have_binlog_format_mixed_or_statement.inc
+# Original test case for bug#51263 needs partitioning.
+--source include/have_partition.inc
 # Save the initial number of concurrent sessions.
 --source include/count_sessions.inc
 
@@ -760,6 +765,104 @@ drop procedure p2;
 drop table t1, t2, t3, t4, t5;
 disconnect con1;
 
+
+--echo #
+--echo # Test for bug#51263 "Deadlock between transactional SELECT
+--echo # and ALTER TABLE ... REBUILD PARTITION".
+--echo #
+connect (con1,localhost,root,,test,,);
+connection default;
+--disable_warnings
+drop table if exists t1, t2;
+--enable_warnings
+create table t1 (i int auto_increment not null primary key) engine=innodb;
+create table t2 (i int) engine=innodb;
+insert into t1 values (1), (2), (3), (4), (5);
+
+begin;
+--echo # Acquire SR metadata lock on t1 and LOCK_S row-locks on its rows.
+insert into t2 select count(*) from t1;
+
+--echo # Switching to connection 'con1'.
+connection con1;
+--echo # Sending:
+--send alter table t1 add column j int
+
+--echo # Switching to connection 'default'.
+connection default;
+--echo # Wait until ALTER is blocked because it tries to upgrade SNW
+--echo # metadata lock to X lock.
+--echo # It should not be blocked during copying data to new version of
+--echo # table as it acquires LOCK_S locks on rows of old version, which
+--echo # are compatible with locks acquired by connection 'con1'.
+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 will deadlock because it will try to acquire
+--echo # SW lock on t1, which will conflict with ALTER's SNW lock. And
+--echo # ALTER will be waiting for this connection to release its SR lock.
+--echo # This deadlock should be detected by an MDL subsystem and this
+--echo # statement should be aborted with an appropriate error.
+--error ER_LOCK_DEADLOCK
+insert into t1 values (6);
+--echo # Unblock ALTER TABLE.
+commit;
+
+--echo # Switching to connection 'con1'.
+connection con1;
+--echo # Reaping ALTER TABLE.
+--reap
+
+--echo # Switching to connection 'default'.
+connection default;
+
+--echo #
+--echo # Now test for scenario in which bug was reported originally.
+--echo #
+drop tables t1, t2;
+create table t1 (i int auto_increment not null primary key) engine=innodb
+  partition by hash (i) partitions 4;
+create table t2 (i int) engine=innodb;
+insert into t1 values (1), (2), (3), (4), (5);
+
+begin;
+--echo # Acquire SR metadata lock on t1.
+select * from t1;
+
+--echo # Switching to connection 'con1'.
+connection con1;
+--echo # Sending:
+--send alter table t1 rebuild partition p0
+
+--echo # Switching to connection 'default'.
+connection default;
+--echo # Wait until ALTER is blocked because of active SR lock.
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Waiting for table" and info = "alter table t1 rebuild partition p0";
+--source include/wait_condition.inc
+
+--echo # The below statement should succeed as transaction
+--echo # has SR metadata lock on t1 and only going to read
+--echo # rows from it.
+insert into t2 select count(*) from t1;
+--echo # Unblock ALTER TABLE.
+commit;
+
+--echo # Switching to connection 'con1'.
+connection con1;
+--echo # Reaping ALTER TABLE.
+--reap
+
+--echo # Switching to connection 'default'.
+connection default;
+disconnect con1;
+--echo # Clean-up.
+drop tables t1, t2;
+
+
 # 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
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index b8831127e3f..cd12e5de4d6 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -1302,7 +1302,7 @@ int ha_partition::prepare_new_partition(TABLE *tbl,
     assumes that external_lock() is last call that may fail here.
     Otherwise see description for cleanup_new_partition().
   */
-  if ((error= file->ha_external_lock(ha_thd(), m_lock_type)))
+  if ((error= file->ha_external_lock(ha_thd(), F_WRLCK)))
     goto error_external_lock;
   DBUG_PRINT("info", ("partition %s external locked", part_name));
 
diff --git a/sql/lock.cc b/sql/lock.cc
index 8e91bd9360e..fbe15fde3b0 100644
--- a/sql/lock.cc
+++ b/sql/lock.cc
@@ -415,7 +415,7 @@ void mysql_unlock_read_tables(THD *thd, MYSQL_LOCK *sql_lock)
   THR_LOCK_DATA **lock=sql_lock->locks;
   for (i=found=0 ; i < sql_lock->lock_count ; i++)
   {
-    if (sql_lock->locks[i]->type >= TL_WRITE_ALLOW_READ)
+    if (sql_lock->locks[i]->type > TL_WRITE_ALLOW_WRITE)
     {
       swap_variables(THR_LOCK_DATA *, *lock, sql_lock->locks[i]);
       lock++;
@@ -435,7 +435,7 @@ void mysql_unlock_read_tables(THD *thd, MYSQL_LOCK *sql_lock)
   for (i=found=0 ; i < sql_lock->table_count ; i++)
   {
     DBUG_ASSERT(sql_lock->table[i]->lock_position == i);
-    if ((uint) sql_lock->table[i]->reginfo.lock_type >= TL_WRITE_ALLOW_READ)
+    if ((uint) sql_lock->table[i]->reginfo.lock_type > TL_WRITE_ALLOW_WRITE)
     {
       swap_variables(TABLE *, *table, sql_lock->table[i]);
       table++;
diff --git a/sql/mdl.cc b/sql/mdl.cc
index ddf518fbb1c..ba938f8714b 100644
--- a/sql/mdl.cc
+++ b/sql/mdl.cc
@@ -1689,9 +1689,8 @@ err:
         shared mode).
 
   @note There can be only one upgrader for a lock or we will have deadlock.
-        This invariant is ensured by code outside of metadata subsystem usually
-        by obtaining some sort of exclusive table-level lock (e.g. TL_WRITE,
-        TL_WRITE_ALLOW_READ) before performing upgrade of metadata lock.
+        This invariant is ensured by the fact that upgradeable locks SNW
+        and SNRW are not compatible with each other and themselves.
 
   @retval FALSE  Success
   @retval TRUE   Failure (thread was killed)
diff --git a/sql/sql_base.cc b/sql/sql_base.cc
index fa484abf0be..4203a556e65 100644
--- a/sql/sql_base.cc
+++ b/sql/sql_base.cc
@@ -5081,8 +5081,8 @@ static bool check_lock_and_start_stmt(THD *thd,
   else
     lock_type= table_list->lock_type;
 
-  if ((int) lock_type >= (int) TL_WRITE_ALLOW_READ &&
-      (int) table_list->table->reginfo.lock_type < (int) TL_WRITE_ALLOW_READ)
+  if ((int) lock_type > (int) TL_WRITE_ALLOW_WRITE &&
+      (int) table_list->table->reginfo.lock_type <= (int) TL_WRITE_ALLOW_WRITE)
   {
     my_error(ER_TABLE_NOT_LOCKED_FOR_WRITE, MYF(0), table_list->alias);
     DBUG_RETURN(1);
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index bad88476c09..c27ebce744f 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -4669,7 +4669,7 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
       To allow concurrent execution of read-only operations we acquire
       weak metadata lock for them.
     */
-    table->mdl_request.set_type((lock_type >= TL_WRITE_ALLOW_READ) ?
+    table->mdl_request.set_type((lock_type >= TL_WRITE_ALLOW_WRITE) ?
                                 MDL_SHARED_NO_READ_WRITE : MDL_SHARED_READ);
     /* open only one table from local list of command */
     {
@@ -7926,7 +7926,7 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list)
   /* Same applies to MDL ticket. */
   table_list->mdl_request.ticket= NULL;
   /* Set lock type which is appropriate for ALTER TABLE. */
-  table_list->lock_type= TL_WRITE_ALLOW_READ;
+  table_list->lock_type= TL_READ_NO_INSERT;
   /* Same applies to MDL request. */
   table_list->mdl_request.set_type(MDL_SHARED_NO_WRITE);
 
diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
index ae09898ada2..e9330574b34 100644
--- a/sql/sql_trigger.cc
+++ b/sql/sql_trigger.cc
@@ -489,7 +489,7 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
   else
   {
     tables->table= open_n_lock_single_table(thd, tables,
-                                            TL_WRITE_ALLOW_READ, 0);
+                                            TL_READ_NO_INSERT, 0);
     if (! tables->table)
       goto end;
     tables->table->use_all_columns();
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index fdc8af942d1..879f65c0fa4 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -697,7 +697,7 @@ static bool add_create_index_prepare (LEX *lex, Table_ident *table)
   lex->sql_command= SQLCOM_CREATE_INDEX;
   if (!lex->current_select->add_table_to_list(lex->thd, table, NULL,
                                               TL_OPTION_UPDATING,
-                                              TL_WRITE_ALLOW_READ,
+                                              TL_READ_NO_INSERT,
                                               MDL_SHARED_NO_WRITE))
     return TRUE;
   lex->alter_info.reset();
@@ -6157,7 +6157,7 @@ alter:
             lex->duplicates= DUP_ERROR; 
             if (!lex->select_lex.add_table_to_list(thd, $4, NULL,
                                                    TL_OPTION_UPDATING,
-                                                   TL_WRITE_ALLOW_READ,
+                                                   TL_READ_NO_INSERT,
                                                    MDL_SHARED_NO_WRITE))
               MYSQL_YYABORT;
             lex->col_list.empty();
@@ -10168,7 +10168,7 @@ drop:
             lex->alter_info.drop_list.push_back(ad);
             if (!lex->current_select->add_table_to_list(lex->thd, $5, NULL,
                                                         TL_OPTION_UPDATING,
-                                                        TL_WRITE_ALLOW_READ,
+                                                        TL_READ_NO_INSERT,
                                                         MDL_SHARED_NO_WRITE))
               MYSQL_YYABORT;
           }
@@ -14094,7 +14094,7 @@ trigger_tail:
             if (!lex->select_lex.add_table_to_list(YYTHD, $9,
                                                    (LEX_STRING*) 0,
                                                    TL_OPTION_UPDATING,
-                                                   TL_WRITE_ALLOW_READ,
+                                                   TL_READ_NO_INSERT,
                                                    MDL_SHARED_NO_WRITE))
               MYSQL_YYABORT;
           }