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

[thavamuni.alagu]

#At file:///home/thava/repo/backup/ based on revid:sanjay.manwani@stripped

 2899 Thava Alagu	2009-12-01
      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.
     @ sql/sql_class.cc
        Fixed race condition in kill query logic. 
        The race condition caused backup_bml_block test failure.

    modified:
      mysql-test/r/truncate_coverage.result
      mysql-test/t/truncate_coverage.test
      sql/sql_class.cc
      storage/myisam/myisam_backup_engine.cc
=== modified file 'mysql-test/r/truncate_coverage.result'
--- a/mysql-test/r/truncate_coverage.result	2009-07-21 16:53:40 +0000
+++ b/mysql-test/r/truncate_coverage.result	2009-12-01 15:38:34 +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-07-21 16:53:40 +0000
+++ b/mysql-test/t/truncate_coverage.test	2009-12-01 15:38:34 +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 'sql/sql_class.cc'
--- a/sql/sql_class.cc	2009-11-26 09:52:48 +0000
+++ b/sql/sql_class.cc	2009-12-01 15:38:34 +0000
@@ -1261,8 +1261,8 @@ void THD::awake(THD::killed_state state_
           pthread_mutex_unlock(mysys_var->current_mutex);
           break;
         }
+        my_sleep(1000000L / WAIT_FOR_KILL_TRY_TIMES);
       }
-      my_sleep(1000000L / WAIT_FOR_KILL_TRY_TIMES);
     }
     pthread_mutex_unlock(&mysys_var->mutex);
   }

=== 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-12-01 15:38:34 +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;
@@ -415,7 +416,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
@@ -458,9 +459,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;
@@ -468,6 +471,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
@@ -478,6 +501,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;
@@ -487,7 +512,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]