Bug#17812505 MULTIPLE ALTER AND SELECT STATEMENTS CAUSING THE SQL NODE TO HANG

For copying ALTER TABLE the server sets table lock TL_READ_NO_INSERT
on the originating table. Use that instead of increasing lock. Ensure
that we lock tuples exclusively for the copying scan table. Adding test
case that verifies there is no more lock conflict.

Approved by : Frazer Clement <frazer.clement@oracle.com>

Author: Martin Sköld

mysql-test/suite/ndb/r/ndb_bug17812505.result

@@ -0,0 +1,20 @@
+CREATE TABLE t1 (
+counter1 integer,
+counter2 integer,
+counter3 integer,
+counter4 integer
+
+) ENGINE=engine;
+call query_select_repeat(5);
+INSERT INTO t1 (counter1, counter2, counter3, counter4)
+VALUES (1,1,1,1);
+INSERT INTO t1 (counter1, counter2, counter3, counter4)
+VALUES (2,2,2,2);
+ALTER TABLE t1 DROP COLUMN counter4;
+INSERT INTO t1 (counter1, counter2, counter3)
+VALUES (3,3,3);
+ALTER TABLE t1 ADD COLUMN counter4 integer;
+INSERT INTO t1 (counter1, counter2, counter3, counter4)
+VALUES (4,4,4,4);
+drop procedure query_select_repeat;
+DROP TABLE t1;

mysql-test/suite/ndb/t/ndb_bug17812505.test

@@ -0,0 +1,84 @@
+--source include/have_ndb.inc
+
+# Select which engine to use(for easy comparison)
+let $engine = NDB;
+#let $engine = InnoDB;
+
+# Choose if table have primary key or not
+# NOTE! Without pk, the table get a hidden primary key
+# which is a "bigint auto increment" when using engine=ndb
+let $table_pk = ;
+#let $table_pk = ,PRIMARY KEY(counter1);
+
+--replace_result $engine engine
+eval CREATE TABLE t1 (
+  counter1 integer,
+  counter2 integer,
+  counter3 integer,
+  counter4 integer
+  $table_pk
+) ENGINE=$engine;
+
+#
+# Create a second connection to the same mysqld
+# to use for running the concurrent select queries
+#
+connect (j1,localhost,root,,test);
+connection j1;
+
+#
+# Create procedure performing select queries
+# repeatedly for specified number of seconds
+#
+--disable_query_log
+delimiter %;
+create procedure query_select_repeat (seconds int)
+begin
+  set @x=time_to_sec(current_time()) + seconds;
+  repeat
+    select counter1,counter2,counter3 from t1;
+  until @x <= time_to_sec(current_time())
+  end repeat;
+end%
+
+delimiter ;%
+--enable_query_log
+
+#
+# Start the select queries to run repeatedly for a couple of seconds
+#
+send call query_select_repeat(5);
+
+#
+# Switch back to default connection and run the ALTERs
+# and INSERTs which locks up with the repeated SELECTs
+#
+connection default;
+INSERT INTO t1 (counter1, counter2, counter3, counter4)
+  VALUES (1,1,1,1);
+INSERT INTO t1 (counter1, counter2, counter3, counter4)
+  VALUES (2,2,2,2);
+ALTER TABLE t1 DROP COLUMN counter4;
+INSERT INTO t1 (counter1, counter2, counter3)
+  VALUES (3,3,3);
+ALTER TABLE t1 ADD COLUMN counter4 integer;
+INSERT INTO t1 (counter1, counter2, counter3, counter4)
+  VALUES (4,4,4,4);
+
+#
+# Wait for the read queries to complete
+#
+connection j1;
+--disable_query_log
+--disable_result_log
+reap;
+--enable_result_log
+--enable_query_log
+
+drop procedure query_select_repeat;
+
+#
+# Switch back to default connection and clean up
+#
+connection default;
+DROP TABLE t1;

sql/ha_ndbcluster.cc

@@ -3556,6 +3556,18 @@ ha_ndbcluster::scan_handle_lock_tuple(NdbScanOperation *scanOp,
   DBUG_RETURN(0);
 }
 
+/*
+  Some MySQL table locks are mapped to Ndb internal exclusive
+  row locks to achieve part of the table locking semantics. If rows are
+  not exclusively locked a new batch of rows need to be fetched.
+ */
+static bool table_lock_not_mapped_to_row_lock(enum thr_lock_type lock_type)
+{
+  return
+    (lock_type < TL_READ_NO_INSERT &&
+     lock_type != TL_READ_WITH_SHARED_LOCKS);
+}
+
 inline int ha_ndbcluster::fetch_next(NdbScanOperation* cursor)
 {
   DBUG_ENTER("fetch_next");
@@ -3567,8 +3579,7 @@ inline int ha_ndbcluster::fetch_next(NdbScanOperation* cursor)
   if ((error= scan_handle_lock_tuple(cursor, trans)) != 0)
     DBUG_RETURN(error);
 
-  bool contact_ndb= m_lock.type < TL_WRITE_ALLOW_WRITE &&
-                    m_lock.type != TL_READ_WITH_SHARED_LOCKS;
+  bool contact_ndb= table_lock_not_mapped_to_row_lock(m_lock.type);
   do {
     DBUG_PRINT("info", ("Call nextResult, contact_ndb: %d", contact_ndb));
     /*
@@ -4352,6 +4363,7 @@ int ha_ndbcluster::full_table_scan(const KEY* key_info,
                                    const key_range *end_key,
                                    uchar *buf)
 {
+  THD *thd= table->in_use;
   int error;
   NdbTransaction *trans= m_thd_ndb->trans;
   part_id_range part_spec;
@@ -4403,7 +4415,14 @@ int ha_ndbcluster::full_table_scan(const KEY* key_info,
     if (unlikely(!(trans= start_transaction(error))))
       DBUG_RETURN(error);
 
-  const NdbOperation::LockMode lm = get_ndb_lock_mode(m_lock.type);
+  /*
+    If the scan is part of an ALTER TABLE we need exclusive locks on rows
+    to block parallel updates from other connections to Ndb.
+  */
+  const NdbOperation::LockMode lm =
+    (thd_sql_command(thd) == SQLCOM_ALTER_TABLE) ?
+    NdbOperation::LM_Exclusive
+    : get_ndb_lock_mode(m_lock.type);
   NdbScanOperation::ScanOptions options;
   options.optionsPresent = (NdbScanOperation::ScanOptions::SO_SCANFLAGS |
                             NdbScanOperation::ScanOptions::SO_PARALLEL);
@@ -8228,22 +8247,15 @@ THR_LOCK_DATA **ha_ndbcluster::store_lock(THD *thd,
     /* In queries of type INSERT INTO t1 SELECT ... FROM t2 ...
        MySQL would use the lock TL_READ_NO_INSERT on t2, and that
        would conflict with TL_WRITE_ALLOW_WRITE, blocking all inserts
-       to t2. Convert the lock to a normal read lock to allow
+       to t2. If table is not explicitly locked or part of an ALTER TABLE
+       then convert the lock to a normal read lock to allow
        concurrent inserts to t2. */
 
-    if (lock_type == TL_READ_NO_INSERT && !thd->in_lock_tables)
+    if (lock_type == TL_READ_NO_INSERT &&
+        !thd->in_lock_tables &&
+        sql_command != SQLCOM_ALTER_TABLE)
       lock_type= TL_READ;
 
-    /**
-     * We need locks on source table when
-     *   doing offline alter...
-     * In 5.1 this worked due to TL_WRITE_ALLOW_READ...
-     * but that has been removed in 5.5
-     * I simply add this to get it...
-     */
-    if (sql_command == SQLCOM_ALTER_TABLE)
-      lock_type = TL_WRITE;
-
     m_lock.type=lock_type;
   }
   *to++= &m_lock;