Follow-up for the fix for bug #46947 "Embedded SELECT without

FOR UPDATE is causing a lock".
 
This patch tries to address problems which were exposed 
during backporting of original patch to 5.1 tree.
 
- It ensures that we don't change locking behavior of simple
  SELECT statements on InnoDB tables when they are executed
  under LOCK TABLES ... READ and with @@innodb_table_locks=0.
  Also we no longer pass TL_READ_DEFAULT/TL_WRITE_DEFAULT 
  lock types, which are supposed to be parser-only, to 
  handler::start_stmt() method.
- It makes check_/no_concurrent_insert.inc auxiliary scripts 
  more robust against changes in test cases that use them
  and also ensures that they don't unnecessarily change 
  environment of caller.

mysql-test/include/check_concurrent_insert.inc

@@ -20,6 +20,9 @@
 --disable_result_log
 --disable_query_log
 
+# Reset DEBUG_SYNC facility for safety.
+set debug_sync= "RESET";
+
 if (`SELECT '$restore_table' <> ''`)
 {
 --eval create table t_backup select * from $restore_table;
@@ -86,5 +89,8 @@ if (`SELECT '$restore_table' <> ''`)
 drop table t_backup;
 }
 
+# Clean-up. Reset DEBUG_SYNC facility after use.
+set debug_sync= "RESET";
+
 --enable_result_log
 --enable_query_log

mysql-test/include/check_no_concurrent_insert.inc

@@ -20,6 +20,9 @@
 --disable_result_log
 --disable_query_log
 
+# Reset DEBUG_SYNC facility for safety.
+set debug_sync= "RESET";
+
 if (`SELECT '$restore_table' <> ''`)
 {
 --eval create table t_backup select * from $restore_table;
@@ -71,5 +74,8 @@ if (`SELECT '$restore_table' <> ''`)
 drop table t_backup;
 }
 
+# Clean-up. Reset DEBUG_SYNC facility after use.
+set debug_sync= "RESET";
+
 --enable_result_log
 --enable_query_log

mysql-test/r/innodb-lock.result

@@ -27,9 +27,10 @@ commit;
 drop table t1;
 #
 # Old lock method (where LOCK TABLE was ignored by InnoDB) no longer
-# works due to fix for bugs  #46272 "MySQL 5.4.4, new MDL: unnecessary
-# deadlock" and bug #37346 "innodb does not detect deadlock between
-# update and alter table".
+# works when LOCK TABLE ... WRITE is used due to fix for bugs #46272
+# "MySQL 5.4.4, new MDL: unnecessary and bug #37346 "innodb does not
+# detect deadlock between update and alter table". But it still works
+# for LOCK TABLE ... READ.
 #
 set @@innodb_table_locks=0;
 create table t1 (id integer primary key, x integer) engine=INNODB;
@@ -61,4 +62,30 @@ commit;
 # Reap LOCK TABLE.
 unlock tables;
 # Connection 'con1'.
+select * from t1 where id = 0 for update;
+id	x
+0	1
+# Connection 'con2'.
+# The below statement should not be blocked as LOCK TABLES ... READ
+# does not take strong SQL-level lock on t1. SELECTs which do not
+# conflict with transaction in the first connections should not be
+# blocked.
+lock table t1 read;
+select * from t1;
+id	x
+0	1
+1	1
+2	2
+select * from t1 where id = 1 lock in share mode;
+id	x
+1	1
+unlock tables;
+select * from t1;
+id	x
+0	1
+1	1
+2	2
+commit;
+# Connection 'con1'.
+commit;
 drop table t1;

mysql-test/t/innodb-lock.test

@@ -58,9 +58,10 @@ drop table t1;
 
 --echo #
 --echo # Old lock method (where LOCK TABLE was ignored by InnoDB) no longer
---echo # works due to fix for bugs  #46272 "MySQL 5.4.4, new MDL: unnecessary
---echo # deadlock" and bug #37346 "innodb does not detect deadlock between
---echo # update and alter table".
+--echo # works when LOCK TABLE ... WRITE is used due to fix for bugs #46272
+--echo # "MySQL 5.4.4, new MDL: unnecessary and bug #37346 "innodb does not
+--echo # detect deadlock between update and alter table". But it still works
+--echo # for LOCK TABLE ... READ.
 --echo #
 
 set @@innodb_table_locks=0;
@@ -102,6 +103,26 @@ unlock tables;
 
 --echo # Connection 'con1'.
 connection con1;
+
+select * from t1 where id = 0 for update;
+
+--echo # Connection 'con2'.
+connection con2;
+--echo # The below statement should not be blocked as LOCK TABLES ... READ
+--echo # does not take strong SQL-level lock on t1. SELECTs which do not
+--echo # conflict with transaction in the first connections should not be
+--echo # blocked.
+lock table t1 read;
+select * from t1;
+select * from t1 where id = 1 lock in share mode;
+unlock tables;
+select * from t1;
+commit;
+
+--echo # Connection 'con1'.
+connection con1;
+commit;
+
 drop table t1;
 
 # End of 4.1 tests

sql/sql_base.cc

@@ -3985,19 +3985,32 @@ recover_from_failed_open(THD *thd, MDL_request *mdl_request,
           replication as the table on the slave might contain other data
           (ie: general_log is enabled on the slave). The statement will
           be marked as unsafe for SBR in decide_logging_format().
+  @remark Note that even in prelocked mode it is important to correctly
+          determine lock type value. In this mode lock type is passed to
+          handler::start_stmt() method and can be used by storage engine,
+          for example, to determine what kind of row locks it should acquire
+          when reading data from the table.
 */
 
 thr_lock_type read_lock_type_for_table(THD *thd,
                                        Query_tables_list *prelocking_ctx,
                                        TABLE_LIST *table_list)
 {
-  bool log_on= mysql_bin_log.is_open() && (thd->variables.option_bits & OPTION_BIN_LOG);
+  /*
+    In cases when this function is called for a sub-statement executed in
+    prelocked mode we can't rely on OPTION_BIN_LOG flag in THD::options
+    bitmap to determine that binary logging is turned on as this bit can
+    be cleared before executing sub-statement. So instead we have to look
+    at THD::sql_log_bin_toplevel member.
+  */
+  bool log_on= mysql_bin_log.is_open() && thd->sql_log_bin_toplevel;
   ulong binlog_format= thd->variables.binlog_format;
   if ((log_on == FALSE) || (binlog_format == BINLOG_FORMAT_ROW) ||
       (table_list->table->s->table_category == TABLE_CATEGORY_LOG) ||
       (table_list->table->s->table_category == TABLE_CATEGORY_PERFORMANCE) ||
       !(is_update_query(prelocking_ctx->sql_command) ||
-        table_list->prelocking_placeholder))
+        table_list->prelocking_placeholder ||
+        (thd->locked_tables_mode > LTM_LOCK_TABLES)))
     return TL_READ;
   else
     return TL_READ_NO_INSERT;
@@ -5001,35 +5014,49 @@ handle_view(THD *thd, Query_tables_list *prelocking_ctx,
 }
 
 
-/*
+/**
   Check that lock is ok for tables; Call start stmt if ok
 
-  SYNOPSIS
-    check_lock_and_start_stmt()
-    thd			Thread handle
-    table_list		Table to check
-    lock_type		Lock used for table
+  @param thd             Thread handle.
+  @param prelocking_ctx  Prelocking context.
+  @param table_list      Table list element for table to be checked.
 
-  RETURN VALUES
-  0	ok
-  1	error
+  @retval FALSE - Ok.
+  @retval TRUE  - Error.
 */
 
-static bool check_lock_and_start_stmt(THD *thd, TABLE *table,
-				      thr_lock_type lock_type)
+static bool check_lock_and_start_stmt(THD *thd,
+                                      Query_tables_list *prelocking_ctx,
+                                      TABLE_LIST *table_list)
 {
   int error;
+  thr_lock_type lock_type;
   DBUG_ENTER("check_lock_and_start_stmt");
 
+  /*
+    TL_WRITE_DEFAULT and TL_READ_DEFAULT are supposed to be parser only
+    types of locks so they should be converted to appropriate other types
+    to be passed to storage engine. The exact lock type passed to the
+    engine is important as, for example, InnoDB uses it to determine
+    what kind of row locks should be acquired when executing statement
+    in prelocked mode or under LOCK TABLES with @@innodb_table_locks = 0.
+  */
+  if (table_list->lock_type == TL_WRITE_DEFAULT)
+    lock_type= thd->update_lock_default;
+  else if (table_list->lock_type == TL_READ_DEFAULT)
+    lock_type= read_lock_type_for_table(thd, prelocking_ctx, table_list);
+  else
+    lock_type= table_list->lock_type;
+
   if ((int) lock_type >= (int) TL_WRITE_ALLOW_READ &&
-      (int) table->reginfo.lock_type < (int) TL_WRITE_ALLOW_READ)
+      (int) table_list->table->reginfo.lock_type < (int) TL_WRITE_ALLOW_READ)
   {
-    my_error(ER_TABLE_NOT_LOCKED_FOR_WRITE, MYF(0),table->alias);
+    my_error(ER_TABLE_NOT_LOCKED_FOR_WRITE, MYF(0), table_list->alias);
     DBUG_RETURN(1);
   }
-  if ((error=table->file->start_stmt(thd, lock_type)))
+  if ((error= table_list->table->file->start_stmt(thd, lock_type)))
   {
-    table->file->print_error(error,MYF(0));
+    table_list->table->file->print_error(error, MYF(0));
     DBUG_RETURN(1);
   }
   DBUG_RETURN(0);
@@ -5174,7 +5201,7 @@ TABLE *open_ltable(THD *thd, TABLE_LIST *table_list, thr_lock_type lock_type,
     table->grant= table_list->grant;
     if (thd->locked_tables_mode)
     {
-      if (check_lock_and_start_stmt(thd, table, lock_type))
+      if (check_lock_and_start_stmt(thd, thd->lex, table_list))
 	table= 0;
     }
     else
@@ -5402,7 +5429,7 @@ bool lock_tables(THD *thd, TABLE_LIST *tables, uint count,
         if (!table->placeholder())
         {
           table->table->query_id= thd->query_id;
-          if (check_lock_and_start_stmt(thd, table->table, table->lock_type))
+          if (check_lock_and_start_stmt(thd, thd->lex, table))
           {
             mysql_unlock_tables(thd, thd->lock);
             thd->lock= 0;
@@ -5456,7 +5483,7 @@ bool lock_tables(THD *thd, TABLE_LIST *tables, uint count,
         }
       }
 
-      if (check_lock_and_start_stmt(thd, table->table, table->lock_type))
+      if (check_lock_and_start_stmt(thd, thd->lex, table))
       {
 	DBUG_RETURN(TRUE);
       }