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

mysys/thr_lock.c

@@ -168,7 +168,8 @@ thr_lock_owner_equal(THR_LOCK_OWNER *rhs, THR_LOCK_OWNER *lhs)
 static uint found_errors=0;
 
 static int check_lock(struct st_lock_list *list, const char* lock_type,
-		      const char *where, my_bool same_owner, my_bool no_cond)
+		      const char *where, my_bool same_owner, my_bool no_cond,
+                      my_bool read_lock)
 {
   THR_LOCK_DATA *data,**prev;
   uint count=0;
@@ -181,6 +182,23 @@ static int check_lock(struct st_lock_list *list, const char* lock_type,
 
     for (data=list->data; data && count++ < MAX_LOCKS ; data=data->next)
     {
+      if (data->type == TL_UNLOCK)
+      {
+	fprintf(stderr,
+		"Warning: Found unlocked lock at %s: %s\n",
+                lock_type, where);
+        return 1;
+      }
+      if ((read_lock && data->type > TL_READ_NO_INSERT) ||
+          (!read_lock && data->type <= TL_READ_NO_INSERT))
+      {
+	fprintf(stderr,
+		"Warning: Found %s lock in %s queue at %s: %s\n",
+                read_lock ? "write" : "read",
+                read_lock ? "read" : "write",
+                lock_type, where);
+        return 1;
+      }
       if (data->type != last_lock_type)
 	last_lock_type=TL_IGNORE;
       if (data->prev != prev)
@@ -237,11 +255,14 @@ static void check_locks(THR_LOCK *lock, const char *where,
 
   if (found_errors < MAX_FOUND_ERRORS)
   {
-    if (check_lock(&lock->write,"write",where,1,1) |
-	check_lock(&lock->write_wait,"write_wait",where,0,0) |
-	check_lock(&lock->read,"read",where,0,1) |
-	check_lock(&lock->read_wait,"read_wait",where,0,0))
+    if (check_lock(&lock->write,"write",where,1,1,0) |
+	check_lock(&lock->write_wait,"write_wait",where,0,0,0) |
+	check_lock(&lock->read,"read",where,0,1,1) |
+	check_lock(&lock->read_wait,"read_wait",where,0,0,1))
+    {
+      DBUG_ASSERT(my_assert_on_error == 0);
       found_errors++;
+    }
 
     if (found_errors < MAX_FOUND_ERRORS)
     {
@@ -592,18 +613,17 @@ wait_for_lock(struct st_lock_list *wait, THR_LOCK_DATA *data,
 
 
 static enum enum_thr_lock_result
-thr_lock(THR_LOCK_DATA *data, THR_LOCK_OWNER *owner,
-         enum thr_lock_type lock_type)
+thr_lock(THR_LOCK_DATA *data, THR_LOCK_OWNER *owner)
 {
   THR_LOCK *lock=data->lock;
   enum enum_thr_lock_result result= THR_LOCK_SUCCESS;
   struct st_lock_list *wait_queue;
   THR_LOCK_DATA *lock_owner;
+  enum thr_lock_type lock_type= data->type;
   DBUG_ENTER("thr_lock");
 
   data->next=0;
   data->cond=0;					/* safety */
-  data->type=lock_type;
   data->owner= owner;                           /* Must be reset ! */
   data->priority&= ~THR_LOCK_LATE_PRIV;
   VOID(pthread_mutex_lock(&lock->mutex));
@@ -912,9 +932,7 @@ void thr_unlock(THR_LOCK_DATA *data, uint unlock_flags)
   if (lock_type == TL_READ_NO_INSERT)
     lock->read_no_write_count--;
   data->type=TL_UNLOCK;				/* Mark unlocked */
-  check_locks(lock,"after releasing lock", lock_type, 1);
   wake_up_waiters(lock);
-  check_locks(lock,"end of thr_unlock", lock_type, 1);
   pthread_mutex_unlock(&lock->mutex);
   DBUG_VOID_RETURN;
 }
@@ -934,6 +952,7 @@ static void wake_up_waiters(THR_LOCK *lock)
   enum thr_lock_type lock_type;
   DBUG_ENTER("wake_up_waiters");
 
+  check_locks(lock, "before waking up waiters", TL_UNLOCK, 1);
   if (!lock->write.data)			/* If no active write locks */
   {
     data=lock->write_wait.data;
@@ -1087,7 +1106,7 @@ thr_multi_lock(THR_LOCK_DATA **data, uint count, THR_LOCK_OWNER *owner)
   /* lock everything */
   for (pos=data,end=data+count; pos < end ; pos++)
   {
-    enum enum_thr_lock_result result= thr_lock(*pos, owner, (*pos)->type);
+    enum enum_thr_lock_result result= thr_lock(*pos, owner);
     if (result != THR_LOCK_SUCCESS)
     {						/* Aborted */
       thr_multi_unlock(data,(uint) (pos-data), 0);

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);
 }
 
 

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

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);
   }
 

sql/sql_handler.cc

@@ -676,7 +676,7 @@ bool mysql_ha_read(THD *thd, TABLE_LIST *tables,
   /* 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,