Bug#18913551 : LOCK TABLES USES INCORRECT LOCK FOR IMPLICITLY

               USED TABLES.

Analysis:
----------
Issue here is, LOCK table operation sometime acquired too weaker
lock on the tables locked implicitly.
LOCK TABLE on a view which uses stored function which uses some
table acquires lock TL_READ on that table. As result, such
LOCK TABLE operation allows concurrent insert on the table
used by function. And this can break statement based replication/
cause wrong binlog order.

While opening tables used in a statement, tables used in stored
functions(if statement uses any) are also opened and locked
implicitly. When statement based replication is on, if function
does not modify any data then the lock TL_READ acquired on such
user tables by ignoring prelocking_placeholder set for them in
function "read_lock_type_for_table". This helps in taking weaker
locks on such tables and allows concurrent insert operations on
them. But when opening tables for "LOCK TABLE" operations, because
of the above mentioned logic, lock type "TL_READ" is set instead 
of "TL_READ_NO_INSERT" for tables of stored function which does
not modify any data. Hence issue mentioned in the report is seen.

Fix:
----------
In function "read_lock_type_for_table", when statement based 
replication is on, for tables of stored functions lock_type 
TL_READ_NO_INSERT is set for LOCK TABLE statement. Flag used to
indicate stored function modifies data or not is ignored in this
case.

Author: phulakun

mysql-test/r/mysqlbinlog_mixed_or_statment.result

@@ -0,0 +1,26 @@
+#
+# Bug#18913551 - LOCK TABLES USES INCORRECT LOCK FOR IMPLICITLY USED
+#                TABLES.
+#
+SET @org_concurrent_insert= @@global.concurrent_insert;
+SET @@global.concurrent_insert=1;
+CREATE TABLE t1(a INT) ENGINE=MyISAM;
+CREATE FUNCTION f1() RETURNS INT RETURN (SELECT MIN(a) FROM t1);
+CREATE VIEW v1 AS (SELECT 1 FROM dual WHERE f1() = 1);
+LOCK TABLE v1 READ;
+connect  con1, localhost, root;
+SET lock_wait_timeout=1;
+# With fix, con1 does not get lock on table "t1" so following insert
+# operation fails after waiting for "lock_wait_timeout" duration.
+INSERT INTO t1 VALUES (1);
+ERROR HY000: Lock wait timeout exceeded; try restarting transaction
+connection default;
+UNLOCK TABLES;
+# V1 should be empty here.
+SELECT * FROM v1;
+1
+disconnect con1;
+SET @@global.concurrent_insert= @org_concurrent_insert;
+DROP TABLE t1;
+DROP VIEW v1;
+DROP FUNCTION f1;

mysql-test/t/mysqlbinlog_mixed_or_statment.test

@@ -0,0 +1,44 @@
+
+--source include/have_binlog_format_mixed_or_statement.inc
+# Save the initial number of concurrent sessions
+--source include/count_sessions.inc
+
+--echo #
+--echo # Bug#18913551 - LOCK TABLES USES INCORRECT LOCK FOR IMPLICITLY USED
+--echo #                TABLES.
+--echo #
+
+--enable_connect_log
+
+SET @org_concurrent_insert= @@global.concurrent_insert;
+SET @@global.concurrent_insert=1;
+
+CREATE TABLE t1(a INT) ENGINE=MyISAM;
+CREATE FUNCTION f1() RETURNS INT RETURN (SELECT MIN(a) FROM t1);
+CREATE VIEW v1 AS (SELECT 1 FROM dual WHERE f1() = 1);
+
+LOCK TABLE v1 READ;
+
+connect (con1, localhost, root);
+SET lock_wait_timeout=1;
+--echo # With fix, con1 does not get lock on table "t1" so following insert
+--echo # operation fails after waiting for "lock_wait_timeout" duration.
+--error ER_LOCK_WAIT_TIMEOUT
+INSERT INTO t1 VALUES (1);
+
+connection default;
+UNLOCK TABLES;
+
+--echo # V1 should be empty here.
+SELECT * FROM v1;
+
+# Cleanup
+disconnect con1;
+SET @@global.concurrent_insert= @org_concurrent_insert;
+DROP TABLE t1;
+DROP VIEW v1;
+DROP FUNCTION f1;
+--disable_connect_log
+
+#Wait till all disconnects are completed
+--source include/wait_until_count_sessions.inc

sql/sql_base.cc

@@ -4239,8 +4239,8 @@ recover_from_failed_open()
           do not modify data but only read it, since in this case nothing is
           written to the binary log. Argument routine_modifies_data
           denotes the same. So effectively, if the statement is not a
-          update query and routine_modifies_data is false, then
-          prelocking_placeholder does not take importance.
+          LOCK TABLE, not a update query and routine_modifies_data is false
+          then prelocking_placeholder does not take importance.
 
           Furthermore, this does not apply to I_S and log tables as it's
           always unsafe to replicate such tables under statement-based
@@ -4267,17 +4267,37 @@ thr_lock_type read_lock_type_for_table(THD *thd,
     at THD::variables::sql_log_bin member.
   */
   bool log_on= mysql_bin_log.is_open() && thd->variables.sql_log_bin;
-  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) ||
+
+  /*
+    When we do not write to binlog or when we use row based replication,
+    it is safe to use a weaker lock.
+  */
+  if (log_on == false ||
+      thd->variables.binlog_format == BINLOG_FORMAT_ROW)
+    return TL_READ;
+
+  if ((table_list->table->s->table_category == TABLE_CATEGORY_LOG) ||
       (table_list->table->s->table_category == TABLE_CATEGORY_RPL_INFO) ||
-      (table_list->table->s->table_category == TABLE_CATEGORY_PERFORMANCE) ||
-      !(is_update_query(prelocking_ctx->sql_command) ||
-        (routine_modifies_data && table_list->prelocking_placeholder) ||
-        (thd->locked_tables_mode > LTM_LOCK_TABLES)))
+      (table_list->table->s->table_category == TABLE_CATEGORY_PERFORMANCE))
     return TL_READ;
-  else
+
+  // SQL queries which updates data need a stronger lock.
+  if (is_update_query(prelocking_ctx->sql_command))
     return TL_READ_NO_INSERT;
+
+  /*
+    table_list is placeholder for prelocking.
+    Ignore prelocking_placeholder status for non "LOCK TABLE" statement's
+    table_list objects when routine_modifies_data is false.
+  */
+  if (table_list->prelocking_placeholder &&
+      (routine_modifies_data || thd->in_lock_tables))
+    return TL_READ_NO_INSERT;
+
+  if (thd->locked_tables_mode > LTM_LOCK_TABLES)
+    return TL_READ_NO_INSERT;
+
+  return TL_READ;
 }