Backport of:

revno: 2476.784.2
committer: davi@moksha.local
timestamp: Thu 2007-09-27 16:56:27 -0300 
message:
Bug#28870 check that table locks are released/reset
    
The problem is that some mysql_lock_tables error paths are not
resetting the tables lock type back to TL_UNLOCK. If the lock
types are not reset properly, a table might be returned to the
table cache with wrong lock_type.
      
The proposed fix is to ensure that the tables lock type is always
properly reset when mysql_lock_tables fails. This is a
incompatible change with respect to the process state information.


sql/lock.cc:
  Merge mysql_lock_tables cleanup sequence and the reset_lock_data
  function into a single function and take steps to ensure it is
  always called for each error exit path. Also remove references
  to the redundant THD::locked variable which was almost exclusively
  used by this function and the same information is already on proc_info.
sql/sql_class.cc:
  Remove references to the THD::locked variable.
sql/sql_class.h:
  Remove the THD::locked variable.
sql/sql_show.cc:
  Remove references to THD:locked, state_info will now default to proc_info.

sql/lock.cc

@@ -90,7 +90,6 @@ extern HASH open_cache;
 
 static MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table,uint count,
 				 uint flags, TABLE **write_locked);
-static void reset_lock_data(MYSQL_LOCK *sql_lock);
 static int lock_external(THD *thd, TABLE **table,uint count);
 static int unlock_external(THD *thd, TABLE **table,uint count);
 static void print_lock_error(int error, const char *);
@@ -194,6 +193,60 @@ int mysql_lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags)
   DBUG_RETURN(0);
 }
 
+
+/**
+  Reset lock type in lock data and free.
+
+  @param mysql_lock Lock structures to reset.
+
+  @note After a locking error we want to quit the locking of the table(s).
+        The test case in the bug report for Bug #18544 has the following
+        cases: 1. Locking error in lock_external() due to InnoDB timeout.
+        2. Locking error in get_lock_data() due to missing write permission.
+        3. Locking error in wait_if_global_read_lock() due to lock conflict.
+
+  @note In all these cases we have already set the lock type into the lock
+        data of the open table(s). If the table(s) are in the open table
+        cache, they could be reused with the non-zero lock type set. This
+        could lead to ignoring a different lock type with the next lock.
+
+  @note Clear the lock type of all lock data. This ensures that the next
+        lock request will set its lock type properly.
+*/
+
+
+static void reset_lock_data(MYSQL_LOCK *sql_lock)
+{
+  THR_LOCK_DATA **ldata, **ldata_end;
+  DBUG_ENTER("reset_lock_data");
+
+  /* Clear the lock type of all lock data to avoid reusage. */
+  for (ldata= sql_lock->locks, ldata_end= ldata + sql_lock->lock_count;
+       ldata < ldata_end;
+       ldata++)
+  {
+    /* Reset lock type. */
+    (*ldata)->type= TL_UNLOCK;
+  }
+  DBUG_VOID_RETURN;
+}
+
+
+/**
+  Reset lock type in lock data and free.
+
+  @param mysql_lock Lock structures to reset.
+
+*/
+
+static void reset_lock_data_and_free(MYSQL_LOCK **mysql_lock)
+{
+  reset_lock_data(*mysql_lock);
+  my_free(*mysql_lock, MYF(0));
+  *mysql_lock= 0;
+}
+
+
 MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
                               uint flags, bool *need_reopen)
 {
@@ -224,16 +277,13 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
       if (wait_if_global_read_lock(thd, 1, 1))
       {
         /* Clear the lock type of all lock data to avoid reusage. */
-        reset_lock_data(sql_lock);
-	my_free((uchar*) sql_lock,MYF(0));
-	sql_lock=0;
+        reset_lock_data_and_free(&sql_lock);
 	break;
       }
       if (thd->version != refresh_version)
       {
         /* Clear the lock type of all lock data to avoid reusage. */
-        reset_lock_data(sql_lock);
-	my_free((uchar*) sql_lock,MYF(0));
+        reset_lock_data_and_free(&sql_lock);
 	goto retry;
       }
     }
@@ -248,9 +298,7 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
 	Someone has issued SET GLOBAL READ_ONLY=1 and we want a write lock.
         We do not wait for READ_ONLY=0, and fail.
       */
-      reset_lock_data(sql_lock);
-      my_free((uchar*) sql_lock, MYF(0));
-      sql_lock=0;
+      reset_lock_data_and_free(&sql_lock);
       my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
       break;
     }
@@ -261,14 +309,11 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
                                                sql_lock->table_count))
     {
       /* Clear the lock type of all lock data to avoid reusage. */
-      reset_lock_data(sql_lock);
-      my_free((uchar*) sql_lock,MYF(0));
-      sql_lock=0;
+      reset_lock_data_and_free(&sql_lock);
       break;
     }
-    thd_proc_info(thd, "Table lock");
     DBUG_PRINT("info", ("thd->proc_info %s", thd->proc_info));
-    thd->locked=1;
+    thd_proc_info(thd, "Locked");
     /* 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));
@@ -281,22 +326,15 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
     {
       if (sql_lock->table_count)
         VOID(unlock_external(thd, sql_lock->table, sql_lock->table_count));
+      reset_lock_data_and_free(&sql_lock);
       my_error(rc, MYF(0));
-      my_free((uchar*) sql_lock,MYF(0));
-      sql_lock= 0;
       break;
     }
     else if (rc == 1)                           /* aborted */
     {
-      /*
-        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);
       thd->some_tables_deleted=1;		// Try again
       sql_lock->lock_count= 0;                  // Locks are already freed
+      // Fall through: unlock, reset lock data, free and retry
     }
     else if (!thd->some_tables_deleted || (flags & MYSQL_LOCK_IGNORE_FLUSH))
     {
@@ -304,23 +342,30 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count,
         Thread was killed or lock aborted. Let upper level close all
         used tables and retry or give error.
       */
-      thd->locked=0;
       break;
     }
     else if (!thd->open_tables)
     {
       // Only using temporary tables, no need to unlock
       thd->some_tables_deleted=0;
-      thd->locked=0;
       break;
     }
     thd_proc_info(thd, 0);
 
-    /* some table was altered or deleted. reopen tables marked deleted */
-    mysql_unlock_tables(thd,sql_lock);
-    thd->locked=0;
+    /* going to retry, unlock all tables */
+    if (sql_lock->lock_count)
+        thr_multi_unlock(sql_lock->locks, sql_lock->lock_count);
+
+    if (sql_lock->table_count)
+      VOID(unlock_external(thd, sql_lock->table, sql_lock->table_count));
+
+    /*
+      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_and_free(&sql_lock);
 retry:
-    sql_lock=0;
     if (flags & MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN)
     {
       *need_reopen= TRUE;
@@ -863,8 +908,7 @@ static MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count,
 	my_error(ER_OPEN_AS_READONLY,MYF(0),table->alias);
         /* Clear the lock type of the lock data that are stored already. */
         sql_lock->lock_count= (uint) (locks - sql_lock->locks);
-        reset_lock_data(sql_lock);
-	my_free((uchar*) sql_lock,MYF(0));
+        reset_lock_data_and_free(&sql_lock);
 	DBUG_RETURN(0);
       }
     }
@@ -905,42 +949,6 @@ static MYSQL_LOCK *get_lock_data(THD *thd, TABLE **table_ptr, uint count,
 }
 
 
-/**
-  Reset lock type in lock data.
-
-  After a locking error we want to quit the locking of the table(s).
-  The test case in the bug report for Bug #18544 has the following
-  cases:
-  -# Locking error in lock_external() due to InnoDB timeout.
-  -# Locking error in get_lock_data() due to missing write permission.
-  -# Locking error in wait_if_global_read_lock() due to lock conflict.
-
-  In all these cases we have already set the lock type into the lock
-  data of the open table(s). If the table(s) are in the open table
-  cache, they could be reused with the non-zero lock type set. This
-  could lead to ignoring a different lock type with the next lock.
-
-  Clear the lock type of all lock data. This ensures that the next
-  lock request will set its lock type properly.
-
-  @param sql_lock                  The MySQL lock.
-*/
-
-static void reset_lock_data(MYSQL_LOCK *sql_lock)
-{
-  THR_LOCK_DATA **ldata;
-  THR_LOCK_DATA **ldata_end;
-
-  for (ldata= sql_lock->locks, ldata_end= ldata + sql_lock->lock_count;
-       ldata < ldata_end;
-       ldata++)
-  {
-    /* Reset lock type. */
-    (*ldata)->type= TL_UNLOCK;
-  }
-}
-
-
 /*****************************************************************************
   Lock table based on the name.
   This is used when we need total access to a closed, not open table

sql/sql_class.cc

@@ -483,7 +483,7 @@ THD::THD()
   catalog= (char*)"std"; // the only catalog we have for now
   main_security_ctx.init();
   security_ctx= &main_security_ctx;
-  locked=some_tables_deleted=no_errors=password= 0;
+  some_tables_deleted=no_errors=password= 0;
   query_start_used= 0;
   count_cuted_fields= CHECK_FIELD_IGNORE;
   killed= NOT_KILLED;

sql/sql_class.h

@@ -1696,7 +1696,7 @@ public:
   bool       slave_thread, one_shot_set;
   /* tells if current statement should binlog row-based(1) or stmt-based(0) */
   bool       current_stmt_binlog_row_based;
-  bool	     locked, some_tables_deleted;
+  bool	     some_tables_deleted;
   bool       last_cuted_field;
   bool	     no_errors, password;
   /**

sql/sql_show.cc

@@ -1707,8 +1707,6 @@ template class I_List<thread_info>;
 
 static const char *thread_state_info(THD *tmp)
 {
-  if (tmp->locked)
-    return "Locked";
 #ifndef EMBEDDED_LIBRARY
   if (tmp->net.reading_or_writing)
   {