bzr commit into mysql-backup-backport branch (ingo.struewing:3024) Bug#47939 WL#5101

[Ingo Struewing]

#At file:///home2/mydev/bzrroot/mysql-5.6-backup-backport-ms09-2/ based on revid:ingo.struewing@stripped

 3024 Ingo Struewing	2010-01-04
      WL#5101 - MySQL Backup back port - MS09
      Merged revid:thavamuni.alagu@stripped
        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.
     @ mysql-test/r/truncate_coverage.result
        WL#5101 - MySQL Backup back port - MS09
        Updated test result.
     @ mysql-test/t/truncate_coverage.test
        WL#5101 - MySQL Backup back port - MS09
        Stabilized test case.
     @ storage/myisam/myisam_backup_engine.cc
        WL#5101 - MySQL Backup back port - MS09
        Moved waiting for the locking thread's end to the Backup destructor.

    modified:
      mysql-test/r/truncate_coverage.result
      mysql-test/t/truncate_coverage.test
      storage/myisam/myisam_backup_engine.cc
=== modified file 'mysql-test/r/truncate_coverage.result'
--- a/mysql-test/r/truncate_coverage.result	2009-12-11 12:24:23 +0000
+++ b/mysql-test/r/truncate_coverage.result	2010-01-04 16:40:18 +0000
@@ -18,10 +18,16 @@ TRUNCATE TABLE t1;
 # connection con1
 SET DEBUG_SYNC='now WAIT_FOR waiting';
 KILL QUERY @id;
-COMMIT;
 #
 # connection default
 ERROR 70100: Query execution was interrupted
+SET DEBUG_SYNC='now SIGNAL interrupt_finished';
+#
+# connection con1
+SET DEBUG_SYNC='now WAIT_FOR interrupt_finished';
+COMMIT;
+#
+# connection default
 UNLOCK TABLES;
 DROP TABLE t1;
 SET DEBUG_SYNC='RESET';

=== modified file 'mysql-test/t/truncate_coverage.test'
--- a/mysql-test/t/truncate_coverage.test	2009-12-11 12:24:23 +0000
+++ b/mysql-test/t/truncate_coverage.test	2010-01-04 16:40:18 +0000
@@ -53,13 +53,23 @@ send TRUNCATE TABLE t1;
 SET DEBUG_SYNC='now WAIT_FOR waiting';
 let $invisible_assignment_in_select = `SELECT @id := $ID`;
 KILL QUERY @id;
-COMMIT;
---disconnect con1
 --echo #
 --echo # connection default
 --connection default
 --error ER_QUERY_INTERRUPTED
 reap;
+SET DEBUG_SYNC='now SIGNAL interrupt_finished';
+
+--echo #
+--echo # connection con1
+--connection con1
+SET DEBUG_SYNC='now WAIT_FOR interrupt_finished';
+COMMIT;
+--disconnect con1
+
+--echo #
+--echo # connection default
+--connection default
 UNLOCK TABLES;
 DROP TABLE t1;
 SET DEBUG_SYNC='RESET';

=== modified file 'storage/myisam/myisam_backup_engine.cc'
--- a/storage/myisam/myisam_backup_engine.cc	2010-01-04 15:46:18 +0000
+++ b/storage/myisam/myisam_backup_engine.cc	2010-01-04 16:40:18 +0000
@@ -269,6 +269,7 @@ private:
   bool cannot_delete_lock_thd;
   pthread_cond_t COND_lock_state; ///< for communication with locking thread
   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;
@@ -412,7 +413,7 @@ void Backup::kill_locking_thread()
 retry:
   pthread_mutex_lock(&THR_LOCK_myisam);
   /* If thread started and not already dead, kill it */
-  if ((lock_state != LOCK_NOT_STARTED) & (lock_state != LOCK_ERROR))
+  if ((lock_state != LOCK_NOT_STARTED) && (lock_state != LOCK_ERROR))
   {
     /*
       If the locking thread has not yet created THD (very unlikely), wait
@@ -455,9 +456,11 @@ 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.
+      The waiting for the confirmation happens at the end.
+    */
   }
   pthread_mutex_unlock(&THR_LOCK_myisam);
   DBUG_VOID_RETURN;
@@ -465,6 +468,26 @@ 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_NOT_STARTED) && (lock_state != LOCK_ERROR))
+    pthread_cond_wait(&COND_lock_state, &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
@@ -475,6 +498,8 @@ retry:
 Backup::~Backup()
 {
   DBUG_ENTER("myisam_backup::Backup::~Backup");
+  kill_locking_thread();
+  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;
@@ -484,7 +509,6 @@ Backup::~Backup()
     delete hash_of_tables;
     hash_of_tables= NULL;
   }
-  kill_locking_thread();
   pthread_cond_destroy(&COND_lock_state);
   DBUG_VOID_RETURN;
 }


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