A 5.1-only version of fix for bug #46947 "Embedded SELECT

without FOR UPDATE is causing a lock".

SELECT statements with subqueries referencing InnoDB tables
were acquiring shared locks on rows in these tables when they
were executed in REPEATABLE-READ mode and with statement or
mixed mode binary logging turned on.

This was a regression which were introduced when fixing
bug 39843.

The problem was that for tables belonging to subqueries
parser set TL_READ_DEFAULT as a lock type. In cases when
statement/mixed binary logging at open_tables() time this
type of lock was converted to TL_READ_NO_INSERT lock at
open_tables() time and caused InnoDB engine to acquire
shared locks on reads from these tables. Although in some
cases such behavior was correct (e.g. for subqueries in
DELETE) in case of SELECT it has caused unnecessary locking.

This patch implements minimal version of the fix for the
specific problem described in the bug-report which supposed
to be not too risky for pushing into 5.1 tree.
The 5.5 tree already contains a more appropriate solution
which also addresses other related issues like bug 53921
"Wrong locks for SELECTs used stored functions may lead
to broken SBR".

This patch tries to solve the problem by ensuring that
TL_READ_DEFAULT lock which is set in the parser for
tables participating in subqueries at open_tables()
time is interpreted as TL_READ_NO_INSERT or TL_READ.
TL_READ is used only if we know that this is a SELECT
and that this particular table is not used by a stored
function.

Test coverage is added for both InnoDB and MyISAM.

This patch introduces an "incompatible" change in locking
scheme for subqueries used in SELECT ... FOR UPDATE and
SELECT .. IN SHARE MODE.

In 4.1 (as well as in 5.0 and 5.1 before fix for bug 39843)
the server would use a snapshot InnoDB read for subqueries
in SELECT FOR UPDATE and SELECT .. IN SHARE MODE statements,
regardless of whether the binary log is on or off.

If the user required a different type of read (i.e. locking
read), he/she could request so explicitly by providing FOR
UPDATE/IN SHARE MODE clause for each individual subquery.

The patch for bug 39843 broke this behaviour (which was not
documented or tested), and started to use locking reads for
all subqueries in SELECT ... FOR UPDATE/IN SHARE MODE.
This patch restores 4.1 behaviour.

This patch should be mostly null-merged into 5.5 tree.

mysql-test/include/check_concurrent_insert.inc:
  Added auxiliary script which allows to check if statement
  reading table allows concurrent inserts in it.
mysql-test/include/check_no_concurrent_insert.inc:
  Added auxiliary script which allows to check that statement
  reading table doesn't allow concurrent inserts in it.
mysql-test/include/check_no_row_lock.inc:
  Added auxiliary script which allows to check if statement
  reading table doesn't take locks on its rows.
mysql-test/include/check_shared_row_lock.inc:
  Added auxiliary script which allows to check if statement
  reading table takes shared locks on some of its rows.
mysql-test/r/bug39022.result:
  After bug #46947 'Embedded SELECT without FOR UPDATE is
  causing a lock' was fixed test case for bug 39022 has to
  be adjusted in order to trigger execution path on which
  original problem was encountered.
mysql-test/r/innodb_mysql_lock2.result:
  Added coverage for handling of locking in various cases when
  we read data from InnoDB tables (includes test case for
  bug #46947 'Embedded SELECT without FOR UPDATE is causing a
  lock').
mysql-test/r/lock_sync.result:
  Added coverage for handling of locking in various cases when
  we read data from MyISAM tables.
mysql-test/t/bug39022.test:
  After bug #46947 'Embedded SELECT without FOR UPDATE is
  causing a lock' was fixed test case for bug 39022 has to
  be adjusted in order to trigger execution path on which
  original problem was encountered.
mysql-test/t/innodb_mysql_lock2.test:
  Added coverage for handling of locking in various cases when
  we read data from InnoDB tables (includes test case for
  bug #46947 'Embedded SELECT without FOR UPDATE is causing a
  lock').
mysql-test/t/lock_sync.test:
  Added coverage for handling of locking in various cases when
  we read data from MyISAM tables.
sql/mysql_priv.h:
  Function read_lock_type_for_table() now takes pointers to
  LEX and TABLE_LIST elements as its arguments since to
  correctly determine lock type it needs to know what
  statement is being performed and whether table element for
  which lock type to be determined belongs to prelocking list.
sql/sql_base.cc:
  Changed read_lock_type_for_table() to return a weak TL_READ
  type of lock in cases when we are executing SELECT (and so
  won't update tables directly) and table doesn't belong to
  statement's prelocking list and thus can't be used by a
  stored function. It is OK to do so since in this case table
  won't be used by statement or function call which will be
  written to the binary log, so serializability requirements
  for it can be relaxed.
  One of results from this change is that SELECTs on InnoDB
  tables no longer takes shared row locks for tables which
  are used in subqueries (i.e. bug #46947 is fixed).
  Another result is that for similar SELECTs on MyISAM tables
  concurrent inserts are allowed.
  In order to implement this change signature of
  read_lock_type_for_table() function was changed to
  take pointers to LEX and TABLE_LIST objects.
sql/sql_update.cc:
  Function read_lock_type_for_table() now takes pointers to
  LEX and TABLE_LIST elements as its arguments since to
  correctly determine lock type it needs to know what
  statement is being performed and whether table element for
  which lock type to be determined belongs to prelocking list.

mysql-test/include/check_concurrent_insert.inc

+#
+# SUMMARY
+#   Check if statement reading table '$table' allows concurrent
+#   inserts in it.
+#
+# PARAMETERS 
+#   $table         Table in which concurrent inserts should be allowed.
+#   $con_aux1      Name of the first auxiliary connection to be used by this
+#                  script.
+#   $con_aux2      Name of the second auxiliary connection to be used by this
+#                  script.
+#   $statement     Statement to be checked.
+#   $restore_table Table which might be modified by statement to be checked
+#                  and thus needs backing up before its execution and
+#                  restoring after it (can be empty).
+#
+# EXAMPLE
+#    lock_sync.test
+#
+--disable_result_log
+--disable_query_log
+
+# Reset DEBUG_SYNC facility for safety.
+set debug_sync= "RESET";
+
+if (`SELECT '$restore_table' <> ''`)
+{
+--eval create temporary table t_backup select * from $restore_table;
+}
+
+connection $con_aux1;
+set debug_sync='after_lock_tables_takes_lock SIGNAL parked WAIT_FOR go';
+--send_eval $statement;
+
+connection $con_aux2;
+set debug_sync='now WAIT_FOR parked';
+--send_eval insert into $table (i) values (0);
+
+--enable_result_log
+--enable_query_log
+connection default;
+# Wait until concurrent insert is successfully executed while
+# statement being checked has its tables locked.
+# We use wait_condition.inc instead of simply reaping 
+# concurrent insert here in order to avoid deadlocks if test
+# fails and to time out gracefully instead.
+let $wait_condition=
+  select count(*) = 0 from information_schema.processlist
+  where info = "insert into $table (i) values (0)";
+--source include/wait_condition.inc
+
+--disable_result_log
+--disable_query_log
+
+if ($success)
+{
+# Apparently concurrent insert was successfully executed.
+# To be safe against wait_condition.inc succeeding due to
+# races let us first reap concurrent insert to ensure that
+# it has really been successfully executed.
+connection $con_aux2;
+--reap
+connection default;
+set debug_sync= 'now SIGNAL go';
+connection $con_aux1;
+--reap
+connection default;
+--echo Success: '$statement' allows concurrent inserts into '$table'.
+}
+if (!$success)
+{
+# Waiting has timed out. Apparently concurrent insert was blocked.
+# So to be able to continue we need to end our statement first.
+set debug_sync= 'now SIGNAL go';
+connection $con_aux1;
+--reap
+connection $con_aux2;
+--reap
+connection default;
+--echo Error: '$statement' doesn't allow concurrent inserts into '$table'!
+}
+
+--eval delete from $table where i = 0;
+
+if (`SELECT '$restore_table' <> ''`)
+{
+--eval truncate table $restore_table;
+--eval insert into $restore_table select * from t_backup;
+drop temporary 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

+#
+# SUMMARY
+#   Check that statement reading table '$table' doesn't allow concurrent
+#   inserts in it.
+#
+# PARAMETERS 
+#   $table         Table in which concurrent inserts should be disallowed.
+#   $con_aux1      Name of the first auxiliary connection to be used by this
+#                  script.
+#   $con_aux2      Name of the second auxiliary connection to be used by this
+#                  script.
+#   $statement     Statement to be checked.
+#   $restore_table Table which might be modified by statement to be checked
+#                  and thus needs backing up before its execution and
+#                  restoring after it (can be empty).
+#
+# EXAMPLE
+#    lock_sync.test
+#
+--disable_result_log
+--disable_query_log
+
+# Reset DEBUG_SYNC facility for safety.
+set debug_sync= "RESET";
+
+if (`SELECT '$restore_table' <> ''`)
+{
+--eval create temporary table t_backup select * from $restore_table;
+}
+
+connection $con_aux1;
+set debug_sync='after_lock_tables_takes_lock SIGNAL parked WAIT_FOR go';
+--send_eval $statement;
+
+connection $con_aux2;
+set debug_sync='now WAIT_FOR parked';
+--send_eval insert into $table (i) values (0);
+
+--enable_result_log
+--enable_query_log
+connection default;
+# Wait until concurrent insert is successfully blocked because
+# of our statement.
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state = "Locked" and info = "insert into $table (i) values (0)";
+--source include/wait_condition.inc
+
+--disable_result_log
+--disable_query_log
+
+set debug_sync= 'now SIGNAL go';
+connection $con_aux1;
+--reap
+connection $con_aux2;
+--reap
+connection default;
+
+if ($success)
+{
+--echo Success: '$statement' doesn't allow concurrent inserts into '$table'.
+}
+if (!$success)
+{
+--echo Error: '$statement' allows concurrent inserts into '$table'!
+}
+
+--eval delete from $table where i = 0;
+
+if (`SELECT '$restore_table' <> ''`)
+{
+--eval truncate table $restore_table;
+--eval insert into $restore_table select * from t_backup;
+drop temporary 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_row_lock.inc

+#
+# SUMMARY
+#   Check if statement affecting or reading table '$table' doesn't
+#   take any kind of locks on its rows.
+#
+# PARAMETERS 
+#   $table      Table for which presence of row locks should be checked.
+#   $con_aux    Name of auxiliary connection to be used by this script.
+#   $statement  Statement to be checked.
+#
+# EXAMPLE
+#    innodb_mysql_lock2.test
+#
+--disable_result_log
+--disable_query_log
+
+connection default;
+begin;
+--eval select * from $table for update;
+
+connection $con_aux;
+begin;
+--send_eval $statement;
+
+--enable_result_log
+--enable_query_log
+
+connection default;
+# Wait until statement is successfully executed while
+# all rows in table are X-locked. This means that it
+# does not acquire any row locks.
+# We use wait_condition.inc instead of simply reaping 
+# statement here in order to avoid deadlocks if test
+# fails and to time out gracefully instead.
+let $wait_condition=
+  select count(*) = 0 from information_schema.processlist
+  where info = "$statement";
+--source include/wait_condition.inc
+
+--disable_result_log
+--disable_query_log
+
+if ($success)
+{
+# Apparently statement was successfully executed and thus it
+# has not required any row locks.
+# To be safe against wait_condition.inc succeeding due to
+# races let us first reap the statement being checked to
+# ensure that it has been successfully executed.
+connection $con_aux;
+--reap
+rollback;
+connection default;
+rollback;
+--echo Success: '$statement' doesn't take row locks on '$table'.
+}
+if (!$success)
+{
+# Waiting has timed out. Apparently statement was blocked on
+# some row lock. So to be able to continue we need to unlock
+# rows first.
+rollback;
+connection $con_aux;
+--reap
+rollback;
+connection default;
+--echo Error: '$statement' takes some row locks on '$table'!
+}
+
+--enable_result_log
+--enable_query_log

mysql-test/include/check_shared_row_lock.inc

+#
+# SUMMARY
+#   Check if statement reading table '$table' takes shared locks
+#   on some of its rows.
+#
+# PARAMETERS 
+#   $table          Table for which presence of row locks should be checked.
+#   $con_aux        Name of auxiliary connection to be used by this script.
+#   $statement      Statement to be checked.
+#   $wait_statement Sub-statement which is supposed to acquire locks (should
+#                   be the same as $statement for ordinary statements).
+#
+# EXAMPLE
+#    innodb_mysql_lock2.test
+#
+--disable_result_log
+--disable_query_log
+
+connection default;
+begin;
+--eval select * from $table for update;
+
+connection $con_aux;
+begin;
+--send_eval $statement;
+
+--enable_result_log
+--enable_query_log
+
+connection default;
+# Wait until statement is successfully blocked because
+# all rows in table are X-locked. This means that at
+# least it acquires S-locks on some of rows.
+let $wait_condition=
+  select count(*) = 1 from information_schema.processlist
+  where state in ("Sending data","statistics", "preparing") and
+        info = "$wait_statement";
+--source include/wait_condition.inc
+
+--disable_result_log
+--disable_query_log
+
+rollback;
+
+connection $con_aux;
+--reap
+rollback;
+
+connection default;
+--enable_result_log
+--enable_query_log
+
+if ($success)
+{
+--echo Success: '$statement' takes shared row locks on '$table'.
+}
+
+if (!$success)
+{
+--echo Error: '$statement' hasn't taken shared row locks on '$table'!
+}

mysql-test/r/bug39022.result

@@ -12,7 +12,7 @@ INSERT INTO t2 VALUES (0),(1),(2),(3),(4),(5),(6),(7),(8),(9),(10),
 START TRANSACTION;
 # in thread2
 REPLACE INTO t2 VALUES (-17);
-SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d);
+SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d) LOCK IN SHARE MODE;
 d
 # in thread1
 REPLACE INTO t1(a,b) VALUES (67,20);
@@ -21,10 +21,10 @@ COMMIT;
 START TRANSACTION;
 REPLACE INTO t1(a,b) VALUES (65,-50);
 REPLACE INTO t2 VALUES (-91);
-SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d);
+SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d) LOCK IN SHARE MODE;
 # in thread1
 # should not crash
-SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d);
+SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d) LOCK IN SHARE MODE;
 ERROR 40001: Deadlock found when trying to get lock; try restarting transaction
 # in thread2
 d

mysql-test/t/bug39022.test

@@ -24,7 +24,7 @@ START TRANSACTION;
 connection thread2;
 --echo # in thread2
 REPLACE INTO t2 VALUES (-17);
-SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d);
+SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d) LOCK IN SHARE MODE;
 
 connection thread1;
 --echo # in thread1
@@ -37,14 +37,14 @@ START TRANSACTION;
 REPLACE INTO t1(a,b) VALUES (65,-50);
 REPLACE INTO t2 VALUES (-91);
 send;
-SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d); #waits
+SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d) LOCK IN SHARE MODE; #waits
 
 connection thread1;
 --echo # in thread1
 
 --echo # should not crash
 --error ER_LOCK_DEADLOCK
-SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d); #crashes
+SELECT d FROM t2,t1 WHERE d=(SELECT MAX(a) FROM t1 WHERE t1.a > t2.d) LOCK IN SHARE MODE; #crashes
 
 connection thread2;
 --echo # in thread2

sql/mysql_priv.h

@@ -1288,7 +1288,8 @@ bool fix_merge_after_open(TABLE_LIST *old_child_list, TABLE_LIST **old_last,
                           TABLE_LIST *new_child_list, TABLE_LIST **new_last);
 bool reopen_table(TABLE *table);
 bool reopen_tables(THD *thd,bool get_locks,bool in_refresh);
-thr_lock_type read_lock_type_for_table(THD *thd, TABLE *table);
+thr_lock_type read_lock_type_for_table(THD *thd, LEX *lex,
+                                       TABLE_LIST *table_list);
 void close_data_files_and_morph_locks(THD *thd, const char *db,
                                       const char *table_name);
 void close_handle_and_leave_table_as_lock(TABLE *table);

sql/sql_base.cc

@@ -4418,7 +4418,8 @@ bool fix_merge_after_open(TABLE_LIST *old_child_list, TABLE_LIST **old_last,
   Return a appropriate read lock type given a table object.
 
   @param thd Thread context
-  @param table TABLE object for table to be locked
+  @param lex        LEX for the current statement.
+  @param table_list Table list element for table to be locked.
 
   @remark Due to a statement-based replication limitation, statements such as
           INSERT INTO .. SELECT FROM .. and CREATE TABLE .. SELECT FROM need
@@ -4427,19 +4428,32 @@ bool fix_merge_after_open(TABLE_LIST *old_child_list, TABLE_LIST **old_last,
           source table. If such a statement gets applied on the slave before
           the INSERT .. SELECT statement finishes, data on the master could
           differ from data on the slave and end-up with a discrepancy between
-          the binary log and table state. Furthermore, this does not apply to
-          I_S and log tables as it's always unsafe to replicate such tables
-          under statement-based 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().
+          the binary log and table state.
+          This also applies to SELECT/SET/DO statements which use stored
+          functions. Calls to such functions are going to be logged as a
+          whole and thus should be serialized against concurrent changes
+          to tables used by those functions. This can be avoided if functions
+          only read data but doing so requires more complex analysis than it
+          is done now (unfortunately, due to bug #53921 "Wrong locks for
+          SELECTs used stored functions may lead to broken SBR" this rule
+          is not followed in cases when stored function or trigger use
+          simple SELECT and not a subselect in their body).
+          Furthermore, this does not apply to I_S and log tables as it's
+          always unsafe to replicate such tables under statement-based
+          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().
 */
 
-thr_lock_type read_lock_type_for_table(THD *thd, TABLE *table)
+thr_lock_type read_lock_type_for_table(THD *thd, LEX *lex,
+                                       TABLE_LIST *table_list)
 {
   bool log_on= mysql_bin_log.is_open() && (thd->options & OPTION_BIN_LOG);
   ulong binlog_format= thd->variables.binlog_format;
   if ((log_on == FALSE) || (binlog_format == BINLOG_FORMAT_ROW) ||
-      (table->s->table_category == TABLE_CATEGORY_PERFORMANCE))
+      (table_list->table->s->table_category == TABLE_CATEGORY_PERFORMANCE) ||
+      (lex->sql_command == SQLCOM_SELECT &&
+       ! table_list->prelocking_placeholder))
     return TL_READ;
   else
     return TL_READ_NO_INSERT;
@@ -4735,7 +4749,7 @@ int open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags)
         tables->table->reginfo.lock_type= thd->update_lock_default;
       else if (tables->lock_type == TL_READ_DEFAULT)
         tables->table->reginfo.lock_type=
-          read_lock_type_for_table(thd, tables->table);
+          read_lock_type_for_table(thd, thd->lex, tables);
       else
         tables->table->reginfo.lock_type= tables->lock_type;
     }
@@ -5389,6 +5403,8 @@ int lock_tables(THD *thd, TABLE_LIST *tables, uint count, bool *need_reopen)
       DBUG_RETURN(-1);
     }
 
+    DEBUG_SYNC(thd, "after_lock_tables_takes_lock");
+
     if (thd->lex->requires_prelocking() &&
         thd->lex->sql_command != SQLCOM_LOCK_TABLES)
     {

sql/sql_update.cc

@@ -1053,7 +1053,7 @@ reopen_tables:
         correct order of statements. Otherwise, we use a TL_READ lock to
         improve performance.
       */
-      tl->lock_type= read_lock_type_for_table(thd, table);
+      tl->lock_type= read_lock_type_for_table(thd, lex, tl);
       tl->updating= 0;
       /* Update TABLE::lock_type accordingly. */
       if (!tl->placeholder() && !using_lock_tables)