bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2897) Bug#47939

[thavamuni.alagu]

#At file:///home/thava/repo/backup/ based on revid:jorgen.loland@stripped

 2897 Thava Alagu	2009-11-27
      Bug#47939 : MyISAM backup driver can wait in unlock() too long
      
      As per the protocol, the backup driver should return from 
      unlock() as early as possible. However, the killing of  
      locking thread by MyISAM driver from within unlock() 
      involves handshakes with locking thread which should
      be eliminated to avoid potential problems.
      
      This is fixed by making kill_locking_thread() not to wait
      for the death of the locking thread.
      The wait for the confirmation of locking thread's death
      happens later at the end.

    modified:
      storage/myisam/myisam_backup_engine.cc
=== modified file 'storage/myisam/myisam_backup_engine.cc'
--- a/storage/myisam/myisam_backup_engine.cc	2009-10-12 09:08:34 +0000
+++ b/storage/myisam/myisam_backup_engine.cc	2009-11-27 08:53:27 +0000
@@ -268,11 +268,14 @@ private:
   THD *lock_thd;
   bool cannot_delete_lock_thd;
   pthread_cond_t COND_lock_state; ///< for communication with locking thread
+  pthread_cond_t COND_lock_death_confirm; ///< To confirm lock thread death.
   void kill_locking_thread();
+  void wait_for_locking_thread_death();
   static const size_t bytes_between_sleeps= 10*1024*1024;
   /** After copying bytes_between_sleeps we sleep sleep_time */
   ulong sleep_time;
   size_t bytes_since_last_sleep; ///< how many bytes sent since we last slept
+  bool should_wait_for_lock_thd_death; ///< Remember to wait for lock thread death.
 };
 
 /* Needed for VisualAge 6.0 */
@@ -392,13 +395,15 @@ result_t Engine::get_backup(const uint32
 Backup::Backup(const Table_list &tables):
   Backup_driver(tables), state(ERROR), image(NULL), stream(1),
   hash_of_tables(NULL), lock_state(LOCK_NOT_STARTED), lock_thd(NULL),
-  cannot_delete_lock_thd(FALSE), bytes_since_last_sleep(0)
+  cannot_delete_lock_thd(FALSE), bytes_since_last_sleep(0),
+  should_wait_for_lock_thd_death(FALSE)
 {
   /*
     Driver is not ready at this point, so state is ERROR.
     This constructor cannot fail, otherwise begin() would have to detect it.
   */
   pthread_cond_init(&COND_lock_state, NULL);
+  pthread_cond_init(&COND_lock_death_confirm, NULL);
 }
 
 
@@ -458,9 +463,12 @@ retry:
     cannot_delete_lock_thd= FALSE;
     /* we wake up thread if it was blocked on the bool above */
     pthread_cond_broadcast(&COND_lock_state);
-    /* And we wait for the thread to inform of its death */
-    while (lock_state != LOCK_ERROR)
-      pthread_cond_wait(&COND_lock_state, &THR_LOCK_myisam);
+    /**
+      This is executed during time critical section, hence we won't
+      wait for the confirmation of the death of locking thread here.
+      We indeed remember to wait later.
+    */
+    should_wait_for_lock_thd_death= TRUE;
   }
   pthread_mutex_unlock(&THR_LOCK_myisam);
   DBUG_VOID_RETURN;
@@ -468,6 +476,25 @@ retry:
 
 
 /**
+  Wait for the death of the locking thread.
+
+  The locking thread is killed using @c kill_locking_thread() method during
+  time sensitive critical section. The confirmation of the death of the
+  locking thread happens little later using this method.
+*/
+
+void Backup::wait_for_locking_thread_death()
+{
+  /* We wait for the locking thread to inform of its death */
+  pthread_mutex_lock(&THR_LOCK_myisam);
+  while (lock_state != LOCK_ERROR){
+    pthread_cond_wait(&COND_lock_death_confirm, &THR_LOCK_myisam);
+  }
+  pthread_mutex_unlock(&THR_LOCK_myisam);
+}
+
+
+/**
   This destructor is only called by the class' free().
   It cleans up any leftover the driver could have. It is safe to call it at
   any point. In a normal (no error) situation, the hash freeing is the only
@@ -478,6 +505,13 @@ retry:
 Backup::~Backup()
 {
   DBUG_ENTER("myisam_backup::Backup::~Backup");
+  kill_locking_thread();
+  /* If we had remembered to wait for the death of locking thread, wait now.*/
+  if (should_wait_for_lock_thd_death)
+  {
+    should_wait_for_lock_thd_death= FALSE;
+    wait_for_locking_thread_death();
+  }
   /* If we had already started backup logging, we must dirtily stop it */
   mi_log(MI_LOG_ACTION_CLOSE_INCONSISTENT, MI_LOG_PHYSICAL, NULL, NULL);
   delete image;
@@ -487,8 +521,8 @@ Backup::~Backup()
     delete hash_of_tables;
     hash_of_tables= NULL;
   }
-  kill_locking_thread();
   pthread_cond_destroy(&COND_lock_state);
+  pthread_cond_destroy(&COND_lock_death_confirm);
   DBUG_VOID_RETURN;
 }
 
@@ -882,7 +916,7 @@ end2:
     pthread_cond_wait(&COND_lock_state, &THR_LOCK_myisam);
   }
   lock_state= LOCK_ERROR;
-  pthread_cond_broadcast(&COND_lock_state);
+  pthread_cond_broadcast(&COND_lock_death_confirm);
   pthread_mutex_unlock(&THR_LOCK_myisam);
   backup::free_table_list(tables_in_TABLE_LIST_form);
   net_end(&thd->net);


Attachment: [text/bzr-bundle] bzr/[e-mail]