Reverted patch for new usage of open_count as it caused more problems than it solved

Cleaned up patch for checking locks for multi-table updates


myisam/mi_close.c:
  Reverted patch for new usage of open_counts
myisam/mi_locking.c:
  Reverted patch for new usage of open_counts
sql/ha_myisam.cc:
  Reverted patch for new usage of open_counts
sql/handler.cc:
  Removed compiler warning
sql/sql_acl.cc:
  Removed compiler warning
sql/sql_table.cc:
  No need to unlock after failed call to external_lock()
sql/sql_update.cc:
  Cleaned up (and made it more secure) patch for checking locks for multi-table updates

7 files changed, 115 insertions, 157 deletions

diff --git a/myisam/mi_close.c b/myisam/mi_close.c
index 47308a5e9eb..2712f0ca283 100644
--- a/myisam/mi_close.c
+++ b/myisam/mi_close.c
@@ -70,8 +70,13 @@ int mi_close(register MI_INFO *info)
       error=my_errno;
     if (share->kfile >= 0)
     {
-      /* We must always flush the state with the current open_count. */
-      if (share->mode != O_RDONLY)
+      /*
+        If we are crashed, we can safely flush the current state as it will
+        not change the crashed state.
+        We can NOT write the state in other cases as other threads
+        may be using the file at this point
+      */
+      if (share->mode != O_RDONLY && mi_is_crashed(info))
 	mi_state_info_write(share->kfile, &share->state, 1);
       if (my_close(share->kfile,MYF(0)))
         error = my_errno;
diff --git a/myisam/mi_locking.c b/myisam/mi_locking.c
index 8a140a8b6fb..1efd203dc5f 100644
--- a/myisam/mi_locking.c
+++ b/myisam/mi_locking.c
@@ -19,16 +19,26 @@
   reads info from a isam-table. Must be first request before doing any furter
   calls to any isamfunktion.  Is used to allow many process use the same
   isamdatabase.
-  */
+*/
+
+/*
+  state.open_count in the .MYI file is used the following way:
+  - For the first change of the file in this process it's incremented with
+    mi_mark_file_change(). (We have a write lock on the file in this case)
+  - In mi_close() it's decremented by _mi_decrement_open_count() if it
+    was incremented in the same process.
+
+  This mean that if we are the only process using the file, the open_count
+  tells us if the MYISAM file wasn't properly closed. (This is true if
+  my_disable_locking is set).
+*/
+
 
 #include "myisamdef.h"
 #ifdef	__WIN__
 #include <errno.h>
 #endif
 
-static int mi_unlock_open_count(MI_INFO *info, my_bool write_info);
-
-
 	/* lock table by F_UNLCK, F_RDLCK or F_WRLCK */
 
 int mi_lock_database(MI_INFO *info, int lock_type)
@@ -38,17 +48,17 @@ int mi_lock_database(MI_INFO *info, int lock_type)
   MYISAM_SHARE *share=info->s;
   uint flag;
   DBUG_ENTER("mi_lock_database");
-  DBUG_PRINT("enter",("mi_lock_database: lock_type %d, old lock %d"
-                      ", r_locks %u, w_locks %u", lock_type,
-                      info->lock_type, share->r_locks, share->w_locks));
-  DBUG_PRINT("enter",("mi_lock_database: gl._changed %d, open_count %u '%s'",
+  DBUG_PRINT("enter",("lock_type: %d  old lock %d  r_locks: %u  w_locks: %u "
+                      "global_changed:  %d  open_count: %u  name: '%s'",
+                      lock_type, info->lock_type, share->r_locks,
+                      share->w_locks,
                       share->global_changed, share->state.open_count,
                       share->index_file_name));
 
   if (share->options & HA_OPTION_READ_ONLY_DATA ||
       info->lock_type == lock_type)
     DBUG_RETURN(0);
-  if (lock_type == F_EXTRA_LCK)
+  if (lock_type == F_EXTRA_LCK)                 /* Used by TMP tables */
   {
     ++share->w_locks;
     ++share->tot_locks;
@@ -90,8 +100,7 @@ int mi_lock_database(MI_INFO *info, int lock_type)
 	  share->state.process= share->last_process=share->this_process;
 	  share->state.unique=   info->last_unique=  info->this_unique;
 	  share->state.update_count= info->last_loop= ++info->this_loop;
-          if (mi_unlock_open_count(info, FALSE) ||
-              mi_state_info_write(share->kfile, &share->state, 1))
+          if (mi_state_info_write(share->kfile, &share->state, 1))
 	    error=my_errno;
 	  share->changed=0;
 	  if (myisam_flush)
@@ -106,19 +115,6 @@ int mi_lock_database(MI_INFO *info, int lock_type)
 	  if (error)
 	    mi_mark_crashed(info);
 	}
-        else
-        {
-          /*
-            There are chances that _mi_mark_file_changed() has been called,
-            while share->changed remained FALSE. Consequently, we need to
-            clear the open_count even when share->changed is FALSE. Note,
-            that mi_unlock_open_count() will only clear the open_count when
-            it is set and only write the status to file, if it changes it
-            and we are running --with-external-locking.
-          */
-          if (mi_unlock_open_count(info, ! my_disable_locking))
-            error= my_errno;
-        }
 	if (info->lock_type != F_EXTRA_LCK)
 	{
 	  if (share->r_locks)
@@ -142,16 +138,17 @@ int mi_lock_database(MI_INFO *info, int lock_type)
       break;
     case F_RDLCK:
       if (info->lock_type == F_WRLCK)
-      {						/* Change RW to READONLY */
+      {
         /*
+          Change RW to READONLY
+
           mysqld does not turn write locks to read locks,
           so we're never here in mysqld.
         */
 	if (share->w_locks == 1)
 	{
 	  flag=1;
-          if (mi_unlock_open_count(info, ! my_disable_locking) ||
-              my_lock(share->kfile,lock_type,0L,F_TO_EOF,
+          if (my_lock(share->kfile,lock_type,0L,F_TO_EOF,
 		      MYF(MY_SEEK_NOT_DONE)))
 	  {
 	    error=my_errno;
@@ -179,14 +176,6 @@ int mi_lock_database(MI_INFO *info, int lock_type)
 	  my_errno=error;
 	  break;
 	}
-        if (share->state.open_count)
-        {
-          DBUG_PRINT("error",("RD: Table has not been correctly unlocked"
-                              ": open_count %d '%s'",
-                              share->state.open_count,
-                              share->index_file_name));
-          mi_mark_crashed(info);
-        }
       }
       VOID(_mi_test_if_changed(info));
       share->r_locks++;
@@ -232,14 +221,6 @@ int mi_lock_database(MI_INFO *info, int lock_type)
 	      my_errno=error;
 	      break;
 	    }
-            if (share->state.open_count)
-            {
-              DBUG_PRINT("error",("WR: Table has not been correctly unlocked"
-                                  ": open_count %d '%s'",
-                                  share->state.open_count,
-                                  share->index_file_name));
-              mi_mark_crashed(info);
-            }
 	  }
 	}
       }
@@ -375,9 +356,10 @@ int _mi_readinfo(register MI_INFO *info, int lock_type, int check_keybuffer)
 } /* _mi_readinfo */
 
 
-	/* Every isam-function that uppdates the isam-database must! end */
-	/* with this request */
-	/* ARGSUSED */
+/*
+  Every isam-function that uppdates the isam-database MUST end with this
+  request
+*/
 
 int _mi_writeinfo(register MI_INFO *info, uint operation)
 {
@@ -450,6 +432,8 @@ int _mi_mark_file_changed(MI_INFO *info)
 {
   char buff[3];
   register MYISAM_SHARE *share=info->s;
+  DBUG_ENTER("_mi_mark_file_changed");
+
   if (!(share->state.changed & STATE_CHANGED) || ! share->global_changed)
   {
     share->state.changed|=(STATE_CHANGED | STATE_NOT_ANALYZED |
@@ -463,12 +447,12 @@ int _mi_mark_file_changed(MI_INFO *info)
     {
       mi_int2store(buff,share->state.open_count);
       buff[2]=1;				/* Mark that it's changed */
-      return (my_pwrite(share->kfile,buff,sizeof(buff),
-			sizeof(share->state.header),
-			MYF(MY_NABP)));
+      DBUG_RETURN(my_pwrite(share->kfile,buff,sizeof(buff),
+                            sizeof(share->state.header),
+                            MYF(MY_NABP)));
     }
   }
-  return 0;
+  DBUG_RETURN(0);
 }
 
 
@@ -501,36 +485,3 @@ int _mi_decrement_open_count(MI_INFO *info)
   }
   return test(lock_error || write_error);
 }
-
-/*
-  Decrement open_count in preparation for unlock.
-
-  SYNOPSIS
-    mi_unlock_open_count()
-    info                        Pointer to the MI_INFO structure.
-    write_info                  If info must be written when changed.
-
-  RETURN
-    0     OK
-*/
-
-static int mi_unlock_open_count(MI_INFO *info, my_bool write_info)
-{
-  int rc= 0;
-  MYISAM_SHARE *share=info->s;
-
-  DBUG_ENTER("mi_unlock_open_count");
-  DBUG_PRINT("enter",("mi_unlock_open_count: gl._changed %d open_count %d '%s'",
-                      share->global_changed, share->state.open_count,
-                      share->index_file_name));
-  if (share->global_changed)
-  {
-    share->global_changed= 0;
-    if (share->state.open_count)
-      share->state.open_count--;
-    if (write_info)
-      rc= _mi_writeinfo(info, WRITEINFO_UPDATE_KEYFILE);
-  }
-  DBUG_RETURN(rc);
-}
-
diff --git a/sql/ha_myisam.cc b/sql/ha_myisam.cc
index 2b7b8f436b1..3bfee7cdd79 100644
--- a/sql/ha_myisam.cc
+++ b/sql/ha_myisam.cc
@@ -998,32 +998,14 @@ int ha_myisam::delete_table(const char *name)
   return mi_delete_table(name);
 }
 
+
 int ha_myisam::external_lock(THD *thd, int lock_type)
 {
-  int rc;
-
-  while ((! (rc= mi_lock_database(file, !table->tmp_table ?
-                                  lock_type : ((lock_type == F_UNLCK) ?
-                                               F_UNLCK : F_EXTRA_LCK)))) &&
-         mi_is_crashed(file) && (myisam_recover_options != HA_RECOVER_NONE))
-  {
-    /*
-      check_and_repair() implicitly write locks the table, unless a
-      LOCK TABLES is in effect. It should be safer to always write lock here.
-      The implicit lock by check_and_repair() will then be a no-op.
-      check_and_repair() does not restore the original lock, but unlocks the
-      table. So we have to get the requested lock type again. And then to 
-      check, if the table has been crashed again meanwhile by another server.
-      If anything fails, we break.
-    */
-    if (((lock_type != F_WRLCK) && (rc= mi_lock_database(file, F_WRLCK))) ||
-        (rc= check_and_repair(thd)))
-      break;
-  }
-  return rc;
+  return mi_lock_database(file, !table->tmp_table ?
+			  lock_type : ((lock_type == F_UNLCK) ?
+				       F_UNLCK : F_EXTRA_LCK));
 }
 
-
 THR_LOCK_DATA **ha_myisam::store_lock(THD *thd,
 				      THR_LOCK_DATA **to,
 				      enum thr_lock_type lock_type)
diff --git a/sql/handler.cc b/sql/handler.cc
index 9eb129fab45..65078a485c5 100644
--- a/sql/handler.cc
+++ b/sql/handler.cc
@@ -348,7 +348,7 @@ int ha_commit_trans(THD *thd, THD_TRANS* trans)
     if (trans == &thd->transaction.all &&
 	my_b_tell(&thd->transaction.trans_log))
     {
-      if (error= wait_if_global_read_lock(thd, 0, 0))
+      if ((error= wait_if_global_read_lock(thd, 0, 0)))
       {
         /*
           Note that ROLLBACK [TO SAVEPOINT] does not have this test; it's
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
index 58d0fe9a7fa..6e782f81ae5 100644
--- a/sql/sql_acl.cc
+++ b/sql/sql_acl.cc
@@ -469,7 +469,7 @@ static ulong get_sort(uint count,...)
     uint chars= 0;
     uint wild_pos= 0;           /* first wildcard position */
 
-    if (start= str)
+    if ((start= str))
     {
       for (; *str ; str++)
       {
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 0dd5c65bf79..6561af9c841 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -2209,11 +2209,7 @@ copy_data_between_tables(TABLE *from,TABLE *to,
     DBUG_RETURN(-1);				/* purecov: inspected */
 
   if (to->file->external_lock(thd, F_WRLCK))
-  {
-    /* We must always unlock, even when lock failed. */
-    (void) to->file->external_lock(thd, F_UNLCK);
     DBUG_RETURN(-1);
-  }
   to->file->extra(HA_EXTRA_WRITE_CACHE);
   from->file->info(HA_STATUS_VARIABLE);
   to->file->deactivate_non_unique_index(from->file->records);
@@ -2313,11 +2309,11 @@ copy_data_between_tables(TABLE *from,TABLE *to,
     error=1;
   if (ha_commit(thd))
     error=1;
+
  err:
   free_io_cache(from);
   *copied= found_count;
   *deleted=delete_count;
-  /* we must always unlock the table on return. */
   if (to->file->external_lock(thd,F_UNLCK))
     error=1;
   DBUG_RETURN(error > 0 ? -1 : 0);
diff --git a/sql/sql_update.cc b/sql/sql_update.cc
index a17742df03b..cdcc90e8651 100644
--- a/sql/sql_update.cc
+++ b/sql/sql_update.cc
@@ -387,6 +387,24 @@ err:
 ***************************************************************************/
 
 /*
+  Get table map for list of Item_field
+*/
+
+static table_map get_table_map(List<Item> *items)
+{
+  List_iterator_fast<Item> item_it(*items);
+  Item_field *item;
+  table_map map= 0;
+
+  while ((item= (Item_field *) item_it++)) 
+    map|= item->used_tables();
+  DBUG_PRINT("info",("table_map: 0x%08x", map));
+  return map;
+}
+
+
+
+/*
   Setup multi-update handling and call SELECT to do the join
 */
 
@@ -401,59 +419,45 @@ int mysql_multi_update(THD *thd,
   int res;
   multi_update *result;
   TABLE_LIST *tl;
-  const bool locked= !(thd->locked_tables);
+  const bool using_lock_tables= thd->locked_tables != 0;
   DBUG_ENTER("mysql_multi_update");
 
+  thd->select_limit= HA_POS_ERROR;
+
   for (;;)
   {
-    table_map update_map= 0;
-    int tnr= 0;
+    table_map update_map;
+    int tnr;
     
     if ((res= open_tables(thd, table_list)))
       DBUG_RETURN(res);
 
-    /*
-      Only need to call lock_tables if (thd->locked_tables == NULL)
-    */
-    if (locked && ((res= lock_tables(thd, table_list))))
+    /* Only need to call lock_tables if we are not using LOCK TABLES */
+    if (!using_lock_tables && ((res= lock_tables(thd, table_list))))
       DBUG_RETURN(res);
 
-    thd->select_limit=HA_POS_ERROR;
-
     /*
       Ensure that we have update privilege for all tables and columns in the
       SET part
       While we are here, initialize the table->map field.
     */
-    for (tl= table_list ; tl ; tl=tl->next)
+    for (tl= table_list,tnr=0 ; tl ; tl=tl->next)
     {
       TABLE *table= tl->table;
       table->grant.want_privilege= (UPDATE_ACL & ~table->grant.privilege);
       table->map= (table_map) 1 << (tnr++);
     }
 
-    if (!setup_fields(thd, table_list, *fields, 1, 0, 0))
-    {
-      List_iterator_fast<Item> field_it(*fields);
-      Item_field *item;
-
-      while ((item= (Item_field *) field_it++)) 
-	update_map|= item->used_tables();
-
-      DBUG_PRINT("info",("update_map=0x%08x", update_map));
-    }
-    else
+    if (setup_fields(thd, table_list, *fields, 1, 0, 0))
       DBUG_RETURN(-1);
 
-    /*
-      Unlock the tables in preparation for relocking
-    */
-    if (locked) 
+    update_map= get_table_map(fields);
+
+    /* Unlock the tables in preparation for relocking */
+    if (!using_lock_tables)
     {      
-      pthread_mutex_lock(&LOCK_open);
       mysql_unlock_tables(thd, thd->lock); 
       thd->lock= 0;
-      pthread_mutex_unlock(&LOCK_open);
     }
 
     /*
@@ -474,26 +478,48 @@ int mysql_multi_update(THD *thd,
 	tl->lock_type= TL_READ;
 	tl->updating= 0;
       }
-      if (locked)
+      if (!using_lock_tables)
 	tl->table->reginfo.lock_type= tl->lock_type;
     }
 
+    /* Relock the tables with the correct modes */
+    res= lock_tables(thd,table_list);
+    if (using_lock_tables)
+    {
+      if (res)
+        DBUG_RETURN(res);
+      break;                                 // Don't have to do setup_field()
+    }
+
     /*
-      Relock the tables
+      We must setup fields again as the file may have been reopened
+      during lock_tables 
     */
-    if (!(res=lock_tables(thd,table_list)))
-      break;
-      
-    if (!locked)
-      DBUG_RETURN(res);
 
-    List_iterator_fast<Item> field_it(*fields);
-    Item_field *item;
+    {
+      List_iterator_fast<Item> field_it(*fields);
+      Item_field *item;
 
-    while ((item= (Item_field *) field_it++)) 
-    /*  item->cleanup();		XXX Use this instead in MySQL 4.1+ */
-      item->field= item->result_field= 0;
+      while ((item= (Item_field *) field_it++)) 
+#if MYSQL_VERSION < 40100
+        item->field= item->result_field= 0;
+#else
+        item->cleanup();
+#endif
+    }
+    if (setup_fields(thd, table_list, *fields, 1, 0, 0))
+      DBUG_RETURN(-1);
+    /*
+      If lock succeded and the table map didn't change since the above lock
+      we can continue.
+    */
+    if (!res && update_map == get_table_map(fields))
+      break;
 
+    /*
+      There was some very unexpected changes in the table definition between
+      open tables and lock tables. Close tables and try again.
+    */
     close_thread_tables(thd);
   }
 
@@ -548,7 +574,7 @@ int multi_update::prepare(List<Item> &not_used_values)
 {
   TABLE_LIST *table_ref;
   SQL_LIST update;
-  table_map tables_to_update= 0;
+  table_map tables_to_update;
   Item_field *item;
   List_iterator_fast<Item> field_it(*fields);
   List_iterator_fast<Item> value_it(*values);
@@ -559,8 +585,7 @@ int multi_update::prepare(List<Item> &not_used_values)
   thd->cuted_fields=0L;
   thd->proc_info="updating main table";
 
-  while ((item= (Item_field *) field_it++))
-    tables_to_update|= item->used_tables();
+  tables_to_update= get_table_map(fields);
 
   if (!tables_to_update)
   {
@@ -624,7 +649,6 @@ int multi_update::prepare(List<Item> &not_used_values)
 
   /* Split fields into fields_for_table[] and values_by_table[] */
 
-  field_it.rewind();
   while ((item= (Item_field *) field_it++))
   {
     Item *value= value_it++;