Fixed lp:933719, "Assertion open_tables == 0 ... " in THD::restore_backup_open_tables_state.

This also fixes a (not likely) crashing bug when forcing a thread that was doing a table lock to re-open it's files, for example by creating a trigger.


mysys/thr_lock.c:
  Added more checking to find wrong locks.
  Removed one, not needed, parameter to thr_lock
sql/lock.cc:
  Fixed mysql_lock_tables() to retry with new sql_lock if lock fails. This was needed as table may be closed and reopened between retry's and then the old sql_lock will point to stale data.
sql/mysql_priv.h:
  Updated prototype
sql/sql_base.cc:
  Ensure that all tables are closed if opening of system table failes; This fixes the assert in THD::restore_backup_open_tables_state
sql/sql_handler.cc:
  Updated variable type

4 files changed, 141 insertions, 110 deletions

diff --git a/sql/lock.cc b/sql/lock.cc
index 3f9b3a9e18b..57ced99417b 100644
--- a/sql/lock.cc
+++ b/sql/lock.cc
@@ -195,31 +195,33 @@ int mysql_lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags)
 MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
                               uint flags, bool *need_reopen)
 {
-  TABLE *write_lock_used;
   MYSQL_LOCK *sql_lock;
+  TABLE *write_lock_used;
+  int rc;
   DBUG_ENTER("mysql_lock_tables(tables)");
 
   *need_reopen= FALSE;
-  if (mysql_lock_tables_check(thd, tables, count, flags))
-    DBUG_RETURN(NULL);
 
-  if (!(sql_lock= get_lock_data(thd, tables, count, GET_LOCK_STORE_LOCKS,
-                                &write_lock_used)) ||
-      ! sql_lock->table_count)
-    DBUG_RETURN(sql_lock);
+  if (mysql_lock_tables_check(thd, tables, count, flags))
+    DBUG_RETURN (NULL);
 
-  if (mysql_lock_tables(thd, sql_lock, write_lock_used != 0, flags,
-                        need_reopen))
+  for (;;)
   {
-    /* Clear the lock type of all lock data to avoid reusage. */
-    reset_lock_data(sql_lock, 1);
+    if (!(sql_lock= get_lock_data(thd, tables, count, GET_LOCK_STORE_LOCKS,
+                                  &write_lock_used)) ||
+        !sql_lock->table_count)
+      break;
+    rc= mysql_lock_tables(thd, sql_lock, write_lock_used != 0, flags,
+                          need_reopen);
+    if (!rc)
+      break;                                    // Got lock
     my_free(sql_lock, MYF(0));
-    sql_lock= 0;
+    if (rc > 0)
+      DBUG_RETURN(0);                           // Failed
   }
   DBUG_RETURN(sql_lock);
 }
 
-
 /**
    Lock a table based on a MYSQL_LOCK structure.
 
@@ -232,120 +234,150 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
    @param need_reopen           Out parameter, TRUE if some tables were altered
                                 or deleted and should be reopened by caller.
 
-   @return 0   ok
-   @return 1  error
+   @return 0  ok
+   @return 1  fatal error
+   @return -1 retry
 */
 
-bool mysql_lock_tables(THD *thd, MYSQL_LOCK *sql_lock,
-                       bool write_lock_used,
-                       uint flags, bool *need_reopen)
+int mysql_lock_tables(THD *thd, MYSQL_LOCK *sql_lock,
+                      bool write_lock_used,
+                      uint flags, bool *need_reopen)
 {
+  int res= 0;
   int rc;
-  bool error= 1;
   DBUG_ENTER("mysql_lock_tables(sql_lock)");
-
   *need_reopen= FALSE;
-  for (;;)
+
+  if (write_lock_used && !(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_LOCK))
   {
-    if (write_lock_used && !(flags & MYSQL_LOCK_IGNORE_GLOBAL_READ_LOCK))
+    if (global_read_lock)
     {
-      if (global_read_lock)
+      /*
+        Someone has issued LOCK ALL TABLES FOR READ and we want a write lock
+        Wait until the lock is gone
+      */
+      if (wait_if_global_read_lock(thd, 1, 1))
       {
-        /*
-          Someone has issued LOCK ALL TABLES FOR READ and we want a write lock
-          Wait until the lock is gone
-        */
-        if (wait_if_global_read_lock(thd, 1, 1))
-          break;
-        if (thd->version != refresh_version)
-          goto retry;
+        /* Clear the lock type of all lock data to avoid reusage. */
+        reset_lock_data(sql_lock, 1);
+        DBUG_RETURN(1);                         // Fatal error
       }
-
-      if (opt_readonly &&
-          !(thd->security_ctx->master_access & SUPER_ACL) &&
-          !thd->slave_thread)
+      if (thd->version != refresh_version)
       {
-        /*
-          Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock.
-          We do not wait for READ_ONLY=0, and fail.
-        */
-        my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
-        break;
+        /* Clear the lock type of all lock data to avoid reusage. */
+        reset_lock_data(sql_lock, 1);
+        goto retry;
       }
     }
 
-    thd_proc_info(thd, "System lock");
-    if (lock_external(thd, sql_lock->table, sql_lock->table_count))
-      break;
-    thd_proc_info(thd, "Table lock");
-    /* Copy the lock data array. thr_multi_lock() reorders its contens. */
-    memcpy(sql_lock->locks + sql_lock->lock_count, sql_lock->locks,
-           sql_lock->lock_count * sizeof(*sql_lock->locks));
-    /* Lock on the copied half of the lock data array. */
-    rc= thr_lock_errno_to_mysql[(int) thr_multi_lock(sql_lock->locks +
-                                                     sql_lock->lock_count,
-                                                     sql_lock->lock_count,
-                                                     thd->lock_id)];
-    if (rc)                                     /* Locking failed */
+    if (opt_readonly &&
+        !(thd->security_ctx->master_access & SUPER_ACL) &&
+        !thd->slave_thread)
     {
+      /*
+        Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock.
+        We do not wait for READ_ONLY=0, and fail.
+      */
+      my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
+      reset_lock_data(sql_lock, 1);
+      DBUG_RETURN(1);                           // Fatal error
+    }
+  }
+
+  thd_proc_info(thd, "System lock");
+  if (lock_external(thd, sql_lock->table, sql_lock->table_count))
+  {
+    /* Clear the lock type of all lock data to avoid reusage. */
+    res= 1;                                     // Fatal error
+    goto end;
+  }
+  thd_proc_info(thd, "Table lock");
+  DBUG_PRINT("info", ("thd->proc_info %s", thd->proc_info));
+  /* Copy the lock data array. thr_multi_lock() reorders its contens. */
+  memcpy(sql_lock->locks + sql_lock->lock_count, sql_lock->locks,
+         sql_lock->lock_count * sizeof(*sql_lock->locks));
+  /* Lock on the copied half of the lock data array. */
+  rc= thr_lock_errno_to_mysql[(int) thr_multi_lock(sql_lock->locks +
+                                                   sql_lock->lock_count,
+                                                   sql_lock->lock_count,
+                                                   thd->lock_id)];
+  if (rc)                                     // Locking failed
+  {
+    if (sql_lock->table_count)
       VOID(unlock_external(thd, sql_lock->table, sql_lock->table_count));
-      if (rc > 1)
-      {
-        /* a timeout or a deadlock */
-        my_error(rc, MYF(0));
-        break;
-      }
-      /* We where aborted and should try again from upper level*/
-      thd->some_tables_deleted= 1;
+
+    /*
+      reset_lock_data is required here. If thr_multi_lock fails it
+      resets lock type for tables, which were locked before (and
+      including) one that caused error. Lock type for other tables
+      preserved.
+    */
+    reset_lock_data(sql_lock, 0);
+
+    if (rc > 1)
+    {
+      my_error(rc, MYF(0));
+      DBUG_RETURN(1);
     }
-    else
+    DBUG_ASSERT(rc == 1);                       // Timeout
+    thd->some_tables_deleted= 1;                // Reopen tables
+    sql_lock->lock_count= 0;                	// Locks are already freed
+    /* Retry */
+  }
+  else
+  {
+    /*
+      Lock worked. Now check that nothing happend while we where waiting
+      to get the lock that would require us to free it.
+    */
+    if (!thd->some_tables_deleted || (flags & MYSQL_LOCK_IGNORE_FLUSH))
+    {
+      res= 0;
+      goto end; /* Lock was not aborted. Return to upper level */
+    }
+    if (!thd->open_tables && !(flags & MYSQL_LOCK_NOT_TEMPORARY))
     {
       /*
-        Lock worked. Now check that nothing happend while we where waiting
-        to get the lock that would require us to free it.
-      */
-      error= 0;
-      if (!thd->some_tables_deleted || (flags & MYSQL_LOCK_IGNORE_FLUSH))
-      {
-        /*
-          Table was not signaled for deletion or we don't care if it was.
-          Return with table as locked.
-        */
-        break;
-      }
-      else if (!thd->open_tables && !(flags & MYSQL_LOCK_NOT_TEMPORARY))
-      {
-        /*
-          Only using temporary tables, no need to unlock.
-          We need the flag as open_tables is not enough to distingush if
-          we are only using temporary tables for tables used trough
-          the HANDLER interface.
+        Only using temporary tables, no need to unlock.
+        We need the flag as open_tables is not enough to distingush if
+        we are only using temporary tables for tables used trough
+        the HANDLER interface.
 
-          We reset some_tables_deleted as it doesn't make sense to have this
-          one when we are only using temporary tables.
-        */
-        thd->some_tables_deleted=0;
-        break;
-      }
-      /* some table was altered or deleted. reopen tables marked deleted */
-      error= 1;
-      mysql_unlock_tables(thd, sql_lock, 0);
+        We reset some_tables_deleted as it doesn't make sense to have this
+        one when we are only using temporary tables.
+      */
+      thd->some_tables_deleted=0;
+      goto end;
     }
+    /* Free lock and retry */
+  }
+
+  /* some table was altered or deleted. reopen tables marked deleted */
+  mysql_unlock_tables(thd, sql_lock, 0);
 
 retry:
-    if (flags & MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN)
-    {
-      *need_reopen= TRUE;
-      break;
-    }
-    if (wait_for_tables(thd))
-      break;                                // Couldn't open tables
-    reset_lock_data(sql_lock, 0);           // Set org locks and retry
+  res= -1;                                      // Retry
+  if (flags & MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN)
+  {
+    *need_reopen= TRUE;                         // Upper level will retry
+    DBUG_RETURN(1);                             // Fatal error
   }
+  if (wait_for_tables(thd))
+    res= 1;                                    // Couldn't open tables
 
+end: 
   thd_proc_info(thd, 0);
+  if (thd->killed)
+  {
+    thd->send_kill_message();
+    if (res == 0)
+      mysql_unlock_tables(thd,sql_lock,0);
+    else
+      reset_lock_data(sql_lock, 1);
+    res= 1;                                     // Fatal
+  }
   thd->set_time_after_lock();
-  DBUG_RETURN(error);
+  DBUG_RETURN(res);
 }
 
 
diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h
index f49b7a3c851..94b06add2bd 100644
--- a/sql/mysql_priv.h
+++ b/sql/mysql_priv.h
@@ -2347,9 +2347,9 @@ extern struct st_VioSSLFd * ssl_acceptor_fd;
 
 MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **table, uint count,
                               uint flags, bool *need_reopen);
-bool mysql_lock_tables(THD *thd, MYSQL_LOCK *sql_lock,
-                       bool write_lock_used,
-                       uint flags, bool *need_reopen);
+int mysql_lock_tables(THD *thd, MYSQL_LOCK *sql_lock,
+                      bool write_lock_used,
+                      uint flags, bool *need_reopen);
 
 /* mysql_lock_tables() and open_table() flags bits */
 #define MYSQL_LOCK_IGNORE_GLOBAL_READ_LOCK      0x0001
diff --git a/sql/sql_base.cc b/sql/sql_base.cc
index 11fd5db2020..95a149741de 100644
--- a/sql/sql_base.cc
+++ b/sql/sql_base.cc
@@ -9606,13 +9606,12 @@ open_performance_schema_table(THD *thd, TABLE_LIST *one_table,
   else
   {
     /*
-      If error in mysql_lock_tables(), open_ltable doesn't close the
-      table. Thread kill during mysql_lock_tables() is such error. But
-      open tables cannot be accepted when restoring the open tables
-      state.
+      This can happen during a thd->kill or while we are trying to log
+      data for a stored procedure/trigger and someone causes the table
+      to be flushed (for example by creating a new trigger for the
+      table)
     */
-    if (thd->killed)
-      close_thread_tables(thd);
+    close_thread_tables(thd);
     thd->restore_backup_open_tables_state(backup);
   }
 
diff --git a/sql/sql_handler.cc b/sql/sql_handler.cc
index d789957ffa4..4f1c63930d6 100644
--- a/sql/sql_handler.cc
+++ b/sql/sql_handler.cc
@@ -676,7 +676,7 @@ retry:
   /* save open_tables state */
   if (handler->lock->lock_count > 0)
   {
-    bool lock_error;
+    int lock_error;
 
     handler->lock->locks[0]->type= handler->lock->locks[0]->org_type;
     lock_error= mysql_lock_tables(thd, handler->lock, 0,