Re: bk commit into 5.2 tree (cbell:1.2610) BUG#31383

[Rafal Somla]

Hi Chuck,

Writing multi thread code is really tricky. I found two other problems which 
might happen in this code. The first one is when kill_locking_thread() is called 
early. Then the folowing race condition might happen:

   [backup kernel thread]
	|	[table locking thread]
	|		|
1  	|	set lock_state to LOCK_NOT_STARTED
	|		|
2 kill_locking_thread()	|
	|		|
3  	|	open_and_lock_tables()
4  	|	set lock_state to LOCK_ACQUIRED
5  	|	<wait on exit condition>
	|		|
6  ~Backup_thread_driver()

When kill_locking_thread() is executed in step 2, lock_state is set to 
LOCK_NOT_STARTED, thus the function will do nothing. As the result the locking 
thread will wait on condition but no one is ever going to broadcast it. When the 
destructor is called, it will wait on COND_driver_wait, since lock_state is now 
LOCK_ACQUIRED which is different from LOCK_DONE. Thus we have a deadlock... :(

I think a good fix will be to remove the "if (lock_state == LOCK_ACQUIRED)" 
condition in kill_locking_thread() and modify the code inside it as follows:

+    pthread_mutex_lock(&THR_LOCK_driver);
+    if (lock_thd && lock_state != LOCK_DONE && lock_state != LOCK_SIGNAL)
+    {
+      lock_state= LOCK_SIGNAL;
+      pthread_mutex_lock(&lock_thd->LOCK_delete);
+      lock_thd->awake(THD::KILL_CONNECTION);
+      pthread_mutex_unlock(&lock_thd->LOCK_delete);
+      pthread_cond_broadcast(&COND_driver_thread_wait);
+    }
+    pthread_mutex_unlock(&THR_LOCK_driver);

So the signalling procedure (setting lock_state to LOCK_SIGNAL and broadcasting 
condition) is executed only if lock_state != NULL (which is the case after the 
locking thread has finished) and lock_state != LOCK_SIGNAL (which means that the 
thread has been already canceled and is not waiting on the condition any more). 
I think it is also better to change the state *before* broadcasting on the 
condition. Also, it is better if lock_state is changed and examined under the 
mutex protection.


The other problem is similar to what we found in the replication thread shutdown 
code. Here is a possible race condition leading to a crash:

   [backup kernel thread]
	|	[table locking thread]
	|		|
2  	|	set lock_state to LOCK_NOT_STARTED
3  	|	open_and_lock_tables()
4  	|	set lock_state to LOCK_ACQUIRED
5  	|	<wait on exit condition>
  	|		|
6 kill_locking_thread()	|
7 (this sets lock_state to LOCK_SIGNAL)
   	|		|
8	|	<wakeup on condition>
9  	|	cleanup (delete thd)
	|	(sets lock_state to LOCK_DONE)
	|		|
10 ~Backup_thread_driver()
	|		|
11	|	broadcast COND_driver_wait - blah!

The problem is that when destructor is called, lock_state is set to LOCK_DONE 
and thus the destructor will not wait on the condition but just destroy all 
mutexes and conditions. But then, in step 11 the locking thread will try to 
broadcast condition which has been deleted - potential crash.

I think an easy fix for this is to switch the order in which COND_driver_wait is 
broadcasted and THR_LOCK_driver mutex is released at the end of 
backup_thread_for_locking() function. That is, it should *first* broadcast on 
the condition and then release the lock.


I believe that after fixing these two issues the code will finally be thread safe.

A suggestion, if you care to refactor the code while fixing above problems and 
agree with the idea. I would move the code which waits for the termination of 
the locking thread from the destructor to the kill_locking_thread() function. 
This way, one will know that after calling kill_locking_thread() the locking 
thread is no longer active. This should make code easier to comprehend. After 
this change, the destructor should call kill_locking_thread() before destroying 
the mutexes and conditions.

Another idea, but this is just for the record and future developement. I wonder 
if it wouldn't be easier to use pthread_join() to wait for the termination of 
the locking thread, instead of using all the condition signaling and waits.

See also below for some small remarks. I wonder if killing the locking thread 
should be signalled as an error or be distinguished from real errors (we have 
similar problem in replication code where termination of a replication thread is 
reported as an error leading to confusing entries in the error log). But I think 
there will be no major problems if it is treated as an error.

This review is getting long and I think you already have something to ponder 
about. I'll look at the rest of the code on next occasion (I can do it also 
while you are working on fixes for the above issues).

Rafal

cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.2 repository of cbell. When cbell does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2007-11-07 16:48:09-05:00, cbell@mysql_cab_desk. +13 -0
>   BUG#31383 : Consistent Snapshot driver not consistent
>   
>   This patch changes the default driver to use a separate thread to open and lock
> tables.
>   The default driver opens tables on the prelock() call from the kernel. The
> snapshot
>   driver initiates the CS read on the lock() call from the kernel and opens tables in
> the first
>   call to get_data() after the lock is taken. This is due to the fact that the
> default driver's
>   validity point is at open_and_lock_tables() while the snapshot driver's validity
> point
>   is at the start of the transaction.
> 
>   mysql-test/r/backup_snapshot.result@stripped, 2007-11-07 16:47:57-05:00,
> cbell@mysql_cab_desk. +75 -10
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     New result file.
> 
>   mysql-test/t/backup_snapshot.test@stripped, 2007-11-07 16:47:58-05:00,
> cbell@mysql_cab_desk. +109 -9
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Test modified to use the get_lock, release_lock functions to stop the CS driver
> in
>     progress allowing simultaneous inserts from the second connection. This patch
> makes this test
>     accurately test the CS driver.
> 
>   sql/backup/CMakeLists.txt@stripped, 2007-11-07 16:47:59-05:00, cbell@mysql_cab_desk. +4
> -1
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added the new be_thread source file and dependency for backup.
> 
>   sql/backup/Makefile.am@stripped, 2007-11-07 16:48:00-05:00, cbell@mysql_cab_desk. +4
> -2
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added the new be_thread source file and dependency for backup.
> 
>   sql/backup/be_default.cc@stripped, 2007-11-07 16:48:00-05:00, cbell@mysql_cab_desk. +46
> -5
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added new methods to support using a separate thread to open and lock tables for
>     backup.
> 
>   sql/backup/be_default.h@stripped, 2007-11-07 16:48:01-05:00, cbell@mysql_cab_desk. +14
> -12
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added new methods to support using a separate thread to open and lock tables for
>     backup.
> 
>   sql/backup/be_snapshot.cc@stripped, 2007-11-07 16:48:02-05:00, cbell@mysql_cab_desk. +29
> -18
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Modifies the CS driver to open and close its own tables while executing in the
>     kernel's thread.
> 
>   sql/backup/be_snapshot.h@stripped, 2007-11-07 16:48:03-05:00, cbell@mysql_cab_desk. +15
> -4
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added methods and variables for opening and closing tables.
> 
>   sql/backup/be_thread.cc@stripped, 2007-11-07 16:48:05-05:00, cbell@mysql_cab_desk. +292
> -0
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added new error message for error handling in threads in default and snapshot
> drivers.
> 
>   sql/backup/be_thread.cc@stripped, 2007-11-07 16:48:05-05:00, cbell@mysql_cab_desk. +0
> -0
> 
>   sql/backup/be_thread.h@stripped, 2007-11-07 16:48:05-05:00, cbell@mysql_cab_desk. +79
> -0
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     New source file for mutex initialization and helper methods for using a thread to
> open
>     and lock tables in default and snapshot drivers.
> 
>   sql/backup/be_thread.h@stripped, 2007-11-07 16:48:05-05:00, cbell@mysql_cab_desk. +0 -0
> 
>   sql/backup/data_backup.cc@stripped, 2007-11-07 16:48:03-05:00, cbell@mysql_cab_desk. +0
> -31
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Removed code to call open and lock tables from kernel.
> 
>   sql/share/errmsg.txt@stripped, 2007-11-07 16:48:04-05:00, cbell@mysql_cab_desk. +3 -0
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added new error message for error handling in threads in the default driver.
> 
>   sql/sql_class.h@stripped, 2007-11-07 16:47:58-05:00, cbell@mysql_cab_desk. +2 -1
>     BUG#31383 : Consistent Snapshot driver not consistent
>     
>     Added a new thread for backup.
> 


> diff -Nrup a/mysql-test/r/backup_snapshot.result
> b/mysql-test/r/backup_snapshot.result
> --- a/mysql-test/r/backup_snapshot.result	2007-07-02 13:42:57 -04:00
> +++ b/mysql-test/r/backup_snapshot.result	2007-11-07 16:47:57 -05:00
> @@ -11,21 +11,73 @@ INSERT INTO bup_snapshot.t1 VALUES ("07 
>  INSERT INTO bup_snapshot.t1 VALUES ("08 Some data to test");
>  INSERT INTO bup_snapshot.t1 VALUES ("09 Some data to test");
>  INSERT INTO bup_snapshot.t1 VALUES ("10 Some data to test");
> +CREATE TABLE bup_snapshot.t2 (a int) ENGINE=MEMORY;
> +INSERT INTO bup_snapshot.t2 VALUES (1), (2), (3), (4), (5);
>  con1: Show that the new data doesn't exist before backup.
>  SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
>  word
>  SELECT COUNT(*) FROM bup_snapshot.t1;
>  COUNT(*)
>  10
> -con1: Backing up database.
> +SELECT COUNT(*) FROM bup_snapshot.t2;
> +COUNT(*)
> +5
> +con2: Getting lock on driver.
> +SELECT get_lock("backup_cs_locked", 100);
> +get_lock("backup_cs_locked", 100)
> +1
> +con1: Backing up database. Spawn this and continue...
>  BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";
> +con2: Wait until backup pauses then insert new data.
> +INSERT INTO bup_snapshot.t1 VALUES("- Dave Mathews");
> +INSERT INTO bup_snapshot.t1 VALUES("- Yes");
> +INSERT INTO bup_snapshot.t1 VALUES("- Jethro Tull");
> +DELETE FROM bup_snapshot.t1 WHERE word LIKE '10%';
> +con2: Showing the data after inserts.
> +SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
> +word
> +- Dave Mathews
> +- Yes
> +- Jethro Tull
> +SELECT COUNT(*) FROM bup_snapshot.t1;
> +COUNT(*)
> +12
> +con2: Release lock on driver.
> +SELECT release_lock("backup_cs_locked");
> +release_lock("backup_cs_locked")
> +1
>  Backup Summary
> - header     =       22 bytes
> - meta-data  =       95 bytes
> - data       =      260 bytes
> + header     =       29 bytes
> + meta-data  =      184 bytes
> + data       =      310 bytes
>                --------------
> - total             377 bytes
> -con2: Inserting new data.
> + total             523 bytes
> +con1: Dropping the database
> +DROP TABLE bup_snapshot.t1;
> +con1: Restoring the database
> +RESTORE FROM "bup_snapshot.bak";
> +Restore Summary
> + header     =       29 bytes
> + meta-data  =      184 bytes
> + data       =      310 bytes
> +              --------------
> + total             523 bytes
> +con1: Showing the data (no new data should be here).
> +SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
> +word
> +SELECT COUNT(*) FROM bup_snapshot.t1;
> +COUNT(*)
> +10
> +SELECT COUNT(*) FROM bup_snapshot.t2;
> +COUNT(*)
> +5
> +con2: Getting lock on driver.
> +SELECT get_lock("backup_cs_reading", 100);
> +get_lock("backup_cs_reading", 100)
> +1
> +con1: Backing up database. Spawn this and continue...
> +BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";
> +con2: Wait until backup pauses then insert new data.
>  INSERT INTO bup_snapshot.t1 VALUES("- Dave Mathews");
>  INSERT INTO bup_snapshot.t1 VALUES("- Yes");
>  INSERT INTO bup_snapshot.t1 VALUES("- Jethro Tull");
> @@ -39,20 +91,33 @@ word
>  SELECT COUNT(*) FROM bup_snapshot.t1;
>  COUNT(*)
>  12
> +con2: Release lock on driver.
> +SELECT release_lock("backup_cs_reading");
> +release_lock("backup_cs_reading")
> +1
> +Backup Summary
> + header     =       29 bytes
> + meta-data  =      184 bytes
> + data       =      310 bytes
> +              --------------
> + total             523 bytes
>  con1: Dropping the database
>  DROP TABLE bup_snapshot.t1;
>  con1: Restoring the database
>  RESTORE FROM "bup_snapshot.bak";
>  Restore Summary
> - header     =       22 bytes
> - meta-data  =       95 bytes
> - data       =      260 bytes
> + header     =       29 bytes
> + meta-data  =      184 bytes
> + data       =      310 bytes
>                --------------
> - total             377 bytes
> + total             523 bytes
>  con1: Showing the data (no new data should be here).
>  SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
>  word
>  SELECT COUNT(*) FROM bup_snapshot.t1;
>  COUNT(*)
>  10
> +SELECT COUNT(*) FROM bup_snapshot.t2;
> +COUNT(*)
> +5
>  DROP DATABASE bup_snapshot;


> diff -Nrup a/mysql-test/t/backup_snapshot.test b/mysql-test/t/backup_snapshot.test
> --- a/mysql-test/t/backup_snapshot.test	2007-07-02 13:42:57 -04:00
> +++ b/mysql-test/t/backup_snapshot.test	2007-11-07 16:47:58 -05:00
> @@ -3,9 +3,19 @@
>  # The test is designed to show that a consistent snapshot
>  # backup can be taken while data is being inserted and deleted.
>  #
> -# TODO
> -#  - Make the test run the insert statements in parallel with the backup
> -#    command using --send and --reap and signals from backup code.
> +# The test is testing the driver to ensure it is entering a 
> +# consistent read state during the backup. There are several
> +# breakpoints in the code that can be used. The two most 
> +# useful ones are:
> +#
> +#   backup_cs_unlock - occurs after consistent read 
> +#     transaction has been started and before the open and
> +#     lock tables.
> +#
> +#   backup_cs_reading - occurs after the open and lock
> +#     tables during the read tables portion.
> +#
> +# The following tests test these conditions.
>  #
>  
>  --source include/have_innodb.inc
> @@ -22,8 +32,11 @@ connect (con2,localhost,root,,);
>  
>  connection con1;
>  
> -# Create a table and load it with lots of data.
> +#
> +# Setup for tests.
> +#
>  
> +# Create a table and load it with lots of data.
>  CREATE TABLE bup_snapshot.t1 (word CHAR(20)) ENGINE=INNODB;
>  
>  INSERT INTO bup_snapshot.t1 VALUES ("01 Some data to test");
> @@ -37,19 +50,42 @@ INSERT INTO bup_snapshot.t1 VALUES ("08 
>  INSERT INTO bup_snapshot.t1 VALUES ("09 Some data to test");
>  INSERT INTO bup_snapshot.t1 VALUES ("10 Some data to test");
>  
> +# Use a non-CS supported table to show driver can coexist with default driver
> +CREATE TABLE bup_snapshot.t2 (a int) ENGINE=MEMORY;
> +INSERT INTO bup_snapshot.t2 VALUES (1), (2), (3), (4), (5);
> +
>  --echo con1: Show that the new data doesn't exist before backup.
>  SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
>  SELECT COUNT(*) FROM bup_snapshot.t1;
> +SELECT COUNT(*) FROM bup_snapshot.t2;
> +
> +connection con2;
> +
> +#
> +# Test 1: Check for consistent read prior to open and lock tables
> +#
> +
> +--echo con2: Getting lock on driver.
> +SELECT get_lock("backup_cs_locked", 100);
>  
>  # While a consistent snapshot backup is executed,
>  # no external inserts should be visible to the transaction.
>  
> ---echo con1: Backing up database.
> -BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";
> +connection con1;
> +
> +--echo con1: Backing up database. Spawn this and continue...
> +send BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";
>  
>  connection con2;
>  
> ---echo con2: Inserting new data.
> +--echo con2: Wait until backup pauses then insert new data.
> +
> +# Must wait to know when backup has entered lock.
> +let $wait_condition = SELECT state = "debug_sync_point: backup_cs_locked"
> +                      FROM INFORMATION_SCHEMA.PROCESSLIST
> +                      WHERE info LIKE "backup database %";
> +--source include/wait_condition.inc
> +
>  INSERT INTO bup_snapshot.t1 VALUES("- Dave Mathews");
>  INSERT INTO bup_snapshot.t1 VALUES("- Yes");
>  INSERT INTO bup_snapshot.t1 VALUES("- Jethro Tull");
> @@ -59,8 +95,13 @@ DELETE FROM bup_snapshot.t1 WHERE word L
>  SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
>  SELECT COUNT(*) FROM bup_snapshot.t1;
>  
> +--echo con2: Release lock on driver.
> +SELECT release_lock("backup_cs_locked");
> +
>  connection con1;
>  
> +reap;
> +
>  # Now restore the database and then check to make sure the new rows
>  # were not backed up.
>  
> @@ -73,9 +114,68 @@ RESTORE FROM "bup_snapshot.bak";
>  --echo con1: Showing the data (no new data should be here).
>  SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
>  SELECT COUNT(*) FROM bup_snapshot.t1;
> +SELECT COUNT(*) FROM bup_snapshot.t2;
>  
> -DROP DATABASE bup_snapshot;
> +remove_file $MYSQLTEST_VARDIR/master-data/bup_snapshot.bak;
> +
> +#
> +# Test 2: Check for consistent read after open and lock tables
> +#
> +
> +connection con2;
> +
> +--echo con2: Getting lock on driver.
> +SELECT get_lock("backup_cs_reading", 100);
> +
> +# While a consistent snapshot backup is executed,
> +# no external inserts should be visible to the transaction.
> +
> +connection con1;
>  
> ---exec rm $MYSQLTEST_VARDIR/master-data/bup_snapshot.bak
> +--echo con1: Backing up database. Spawn this and continue...
> +send BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";
> +
> +connection con2;
> +
> +--echo con2: Wait until backup pauses then insert new data.
> +
> +# Must wait to know when backup has entered lock.
> +let $wait_condition = SELECT state = "debug_sync_point: backup_cs_reading"
> +                      FROM INFORMATION_SCHEMA.PROCESSLIST
> +                      WHERE info LIKE "backup database %";
> +--source include/wait_condition.inc
> +
> +INSERT INTO bup_snapshot.t1 VALUES("- Dave Mathews");
> +INSERT INTO bup_snapshot.t1 VALUES("- Yes");
> +INSERT INTO bup_snapshot.t1 VALUES("- Jethro Tull");
> +DELETE FROM bup_snapshot.t1 WHERE word LIKE '10%';
> +
> +--echo con2: Showing the data after inserts.
> +SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
> +SELECT COUNT(*) FROM bup_snapshot.t1;
> +
> +--echo con2: Release lock on driver.
> +SELECT release_lock("backup_cs_reading");
> +
> +connection con1;
> +
> +reap;
> +
> +# Now restore the database and then check to make sure the new rows
> +# were not backed up.
> +
> +--echo con1: Dropping the database
> +DROP TABLE bup_snapshot.t1;
> +
> +--echo con1: Restoring the database
> +RESTORE FROM "bup_snapshot.bak";
> +
> +--echo con1: Showing the data (no new data should be here).
> +SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
> +SELECT COUNT(*) FROM bup_snapshot.t1;
> +SELECT COUNT(*) FROM bup_snapshot.t2;
> +
> +DROP DATABASE bup_snapshot;
>  
> +remove_file $MYSQLTEST_VARDIR/master-data/bup_snapshot.bak;
>  


> diff -Nrup a/sql/backup/CMakeLists.txt b/sql/backup/CMakeLists.txt
> --- a/sql/backup/CMakeLists.txt	2007-10-03 12:56:57 -04:00
> +++ b/sql/backup/CMakeLists.txt	2007-11-07 16:47:59 -05:00
> @@ -25,8 +25,11 @@ INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/
>  SET(BACKUP_SOURCES stream.cc logger.cc string_pool.cc
>                 archive.cc meta_backup.cc data_backup.cc
>                 sql_backup.cc be_default.cc buffer_iterator.cc
> -               be_snapshot.cc)
> +               be_snapshot.cc be_thread.cc)
>  
>  IF(NOT SOURCE_SUBLIBS)
>    ADD_LIBRARY(backup ${BACKUP_SOURCES})
>  ENDIF(NOT SOURCE_SUBLIBS)
> +
> +ADD_DEPENDENCIES(backup mysys)
> +


> diff -Nrup a/sql/backup/Makefile.am b/sql/backup/Makefile.am
> --- a/sql/backup/Makefile.am	2007-10-05 09:12:28 -04:00
> +++ b/sql/backup/Makefile.am	2007-11-07 16:48:00 -05:00
> @@ -33,7 +33,8 @@ libbackup_la_SOURCES = \
>  	sql_backup.cc \
>        be_default.cc \
>        be_snapshot.cc \
> -      buffer_iterator.cc
> +      buffer_iterator.cc \
> +      be_thread.cc
>  
>  noinst_HEADERS = \
>  	api_types.h \
> @@ -50,7 +51,8 @@ noinst_HEADERS = \
>      meta_backup.h \
>        be_default.h \
>        be_snapshot.h \
> -      buffer_iterator.h
> +      buffer_iterator.h \
> +      be_thread.h
>  
>  DEFS = \
>  	-DMYSQL_SERVER \


> diff -Nrup a/sql/backup/be_default.cc b/sql/backup/be_default.cc
> --- a/sql/backup/be_default.cc	2007-07-02 13:42:58 -04:00
> +++ b/sql/backup/be_default.cc	2007-11-07 16:48:00 -05:00
> @@ -108,14 +108,14 @@ result_t Engine::get_backup(const uint32
>  }
>  
>  Backup::Backup(const Table_list &tables, THD *t_thd, thr_lock_type lock_type): 
> -               Backup_driver(tables)
> +               Backup_thread_driver(tables)
>  {
>    DBUG_PRINT("default_backup",("Creating backup driver"));
>    m_thd= t_thd;         /* save current thread */
>    cur_table= NULL;      /* flag current table as null */
>    tbl_num= 0;           /* set table number to 0 */
>    mode= INITIALIZE;     /* initialize read */
> -  lock_called= FALSE;   /* lock has not been called */
> +  lock_thd= NULL;       /* set lock thread to 0 */
>  
>    /*
>       Create a TABLE_LIST * list for iterating through the tables.
> @@ -123,6 +123,21 @@ Backup::Backup(const Table_list &tables,
>    */
>    tables_in_backup= build_table_list(tables, lock_type);
>    all_tables= tables_in_backup;
> +  init_phase_complete= FALSE;
> +  locks_acquired= FALSE;
> +}
> +
> +/**
> +  * @brief Prelock call to setup locking.
> +  *
> +  * Launches a separate thread ("locking thread") which will lock
> +  * tables. Locking in a separate thread is needed to have a non-blocking
> +  * prelock() (given that thr_lock() is blocking).
> +  */
> +result_t Backup::prelock()
> +{
> +  DBUG_ENTER("Default_backup::prelock()");
> +  DBUG_RETURN(start_locking_thread());
>  }
>  
>  /**
> @@ -232,14 +247,33 @@ result_t Backup::get_data(Buffer &buf)
>    DBUG_ENTER("Default_backup::get_data(Buffer &buf)");
>  
>    /*
> -    If locks have not been obtained, wait until they have.
> +    Check the lock state. Take action based on the availability of the lock.
> +
> +    @todo Refactor the following code to make this a new mode for the driver,
> +          e.g. INIT_PHASE, SYNCH_PHASE, ACQUIRING_LOCKS, etc.
>    */
> -  if (!lock_called)
> +  if (!locks_acquired)
>    {
>      buf.size= 0;
>      buf.table_no= 0; 
>      buf.last= TRUE;
> -    DBUG_RETURN(READY);
> +    switch (lock_state) {
> +    case LOCK_ERROR:             // Something ugly happened in locking
> +      DBUG_RETURN(ERROR);
> +    case LOCK_ACQUIRED:          // First time lock ready for validity point
> +    {
> +      locks_acquired= TRUE;
> +      DBUG_RETURN(READY);
> +    }
> +    default:                     // If first call, signal end of init phase
> +      if (init_phase_complete)
> +        DBUG_RETURN(OK);
> +      else
> +      {
> +        init_phase_complete= TRUE;
> +        DBUG_RETURN(READY);
> +      }
> +    }
>    }
>  
>    buf.table_no= tbl_num;
> @@ -304,6 +338,13 @@ result_t Backup::get_data(Buffer &buf)
>        buf.size= 0;
>        buf.last= TRUE;
>        mode= GET_NEXT_TABLE;
> +
> +      /*
> +        Optimization: If this is the last table to read, close the tables and
> +        kill the lock thread. This only applies iff we are using the thread.
> +      */
> +      if (tables_in_backup->next_global == NULL)
> +        kill_locking_thread();
>      }
>      else if (last_read_res != 0)
>        DBUG_RETURN(ERROR);


> diff -Nrup a/sql/backup/be_default.h b/sql/backup/be_default.h
> --- a/sql/backup/be_default.h	2007-07-02 13:42:58 -04:00
> +++ b/sql/backup/be_default.h	2007-11-07 16:48:01 -05:00
> @@ -6,6 +6,8 @@
>  #include "archive.h"
>  #include "buffer_iterator.h"
>  #include "backup_aux.h"
> +#include "mysql_priv.h"
> +#include "be_thread.h"
>  
>  namespace default_backup {
>  
> @@ -78,32 +80,34 @@ class Engine: public Backup_engine
>   * a table scan on each table reading the rows and saving the data to the
>   * buffer from the backup algorithm.
>   *
> - * @see <backup driver>
> + * @see <backup driver> and <backup thread driver>
>   */
> -class Backup: public Backup_driver
> +class Backup: public Backup_thread_driver
>  {
>    public:
>      enum has_data_info { YES, WAIT, EOD };
>      Backup(const Table_list &tables, THD *t_thd, thr_lock_type lock_type);
> -    virtual ~Backup() { backup::free_table_list(all_tables); }; 
> +    virtual ~Backup() 
> +    {
> +      kill_locking_thread();
> +      backup::free_table_list(all_tables);
> +    }; 
>      size_t size()  { return UNKNOWN_SIZE; };
>      size_t init_size() { return 0; };
>      result_t  begin(const size_t) { return backup::OK; };
>      result_t end() { return backup::OK; };
>      result_t get_data(Buffer &buf);
> -    result_t lock() 
> -    {
> -      lock_called= TRUE;
> -      return backup::OK; 
> -    };
> +    result_t lock() { return backup::OK; };
>      result_t unlock() { return backup::OK; };
>      result_t cancel() { return backup::OK; };
>      TABLE_LIST *get_table_list() { return all_tables; }
>      void free() { delete this; };
> +    result_t prelock(); 
>  
>   protected:
> -    my_bool lock_called;           ///< Checks to see if locks have been
> reached.
> -    THD *m_thd;                    ///< Pointer to current thread struct.
> +    TABLE *cur_table;              ///< The table currently being read.
> +    my_bool init_phase_complete;   ///< Used to identify end of init phase.
> +    my_bool locks_acquired;        ///< Used to help kernel synchronize drivers.
>  
>    private:
>      /*
> @@ -126,7 +130,6 @@ class Backup: public Backup_driver
>      int next_table();
>      BACKUP_MODE mode;              ///< Indicates which mode the code is in
>      int tbl_num;                   ///< The index of the current table.
> -    TABLE *cur_table;              ///< The table currently being read.
>      handler *hdl;                  ///< Pointer to table handler.
>      uint *cur_blob;                ///< The current blob field.
>      uint *last_blob_ptr;           ///< Position of last blob field.
> @@ -135,7 +138,6 @@ class Backup: public Backup_driver
>      Buffer_iterator blob_buffer;   ///< Buffer iterator for windowing BLOB
> fields
>      byte *ptr;                     ///< Pointer to blob data from record.
>      TABLE_LIST *all_tables;        ///< Reference to list of tables used.
> -    TABLE_LIST *tables_in_backup;  ///< List of tables used in backup.
>  };
>  
>  /**


> diff -Nrup a/sql/backup/be_snapshot.cc b/sql/backup/be_snapshot.cc
> --- a/sql/backup/be_snapshot.cc	2007-07-02 13:42:59 -04:00
> +++ b/sql/backup/be_snapshot.cc	2007-11-07 16:48:02 -05:00
> @@ -77,20 +77,6 @@ result_t Engine::get_backup(const uint32
>    DBUG_RETURN(OK);
>  }
>  
> -/**
> - * @brief End backup process.
> - *
> - * This method unlocks all of the tables.
> - *
> - * @retval backup::OK    all tables unlocked.
> - */
> -result_t Backup::end()
> -{
> -  DBUG_ENTER("Snapshot_backup::end");
> -  end_active_trans(m_thd);
> -  DBUG_RETURN(OK);
> -}
> -
>  result_t Backup::lock()
>  {
>    DBUG_ENTER("Snapshot_backup::lock()");
> @@ -104,14 +90,39 @@ result_t Backup::lock()
>    int res= begin_trans(m_thd);
>    if (res)
>      DBUG_RETURN(ERROR);
> -  lock_called= TRUE;
> +  lock_state= LOCK_ACQUIRED;
> +  BACKUP_SYNC("backup_cs_locked");
>    DBUG_RETURN(OK);
>  }
>  
> -result_t Backup::unlock()
> +result_t Backup::get_data(Buffer &buf)
>  {
> -  DBUG_ENTER("Snapshot_backup::unlock()");
> -  DBUG_RETURN(OK);
> +  result_t res;
> +
> +  if (!tables_open && (lock_state == LOCK_ACQUIRED))
> +  {
> +    BACKUP_SYNC("backup_cs_open_tables");
> +    open_and_lock_tables(m_thd, tables_in_backup);
> +    tables_open= TRUE;
> +  }
> +  if (lock_state == LOCK_ACQUIRED)
> +    BACKUP_SYNC("backup_cs_reading");
> +
> +  res= default_backup::Backup::get_data(buf);
> +
> +  /*
> +    If this is the last table to be read, close the transaction
> +    and unlock the tables. This is indicated by the lock state
> +    being set to LOCK_SIGNAL from parent::get_data(). This is set
> +    after the last table is finished reading.
> +  */
> +  if (lock_state == LOCK_SIGNAL)
> +  {
> +    lock_state= LOCK_DONE;     // set lock done so destructor won't wait
> +    end_active_trans(m_thd);
> +    close_thread_tables(m_thd);
> +  }
> +  return(res);
>  }
>  
>  /**


> diff -Nrup a/sql/backup/be_snapshot.h b/sql/backup/be_snapshot.h
> --- a/sql/backup/be_snapshot.h	2007-07-02 13:42:59 -04:00
> +++ b/sql/backup/be_snapshot.h	2007-11-07 16:48:03 -05:00
> @@ -59,13 +59,24 @@ class Backup: public default_backup::Bac
>  {
>    public:
>      Backup(const Table_list &tables, THD *t_thd): 
> -      default_backup::Backup(tables, t_thd, TL_READ) {};
> -    virtual ~Backup() {};
> +      default_backup::Backup(tables, t_thd, TL_READ) { tables_open= FALSE; };
> +    virtual ~Backup()
> +    {
> +      if (lock_state == LOCK_ACQUIRED)
> +      {
> +        end_active_trans(m_thd);
> +        close_thread_tables(m_thd);
> +      }
> +    };
>      result_t begin(const size_t) { return backup::OK; };
> -    result_t end();
> +    result_t end() { return backup::OK; };
> +    result_t get_data(Buffer &buf);
>      result_t prelock() { return backup::READY; }
>      result_t lock();
> -    result_t unlock();
> +    result_t unlock() { return backup::OK; };
> +    result_t cancel() { return backup::OK; };
> +  private:
> +    my_bool tables_open;   ///< Indicates if tables are open
>  };
>  
>  /**


> diff -Nrup a/sql/backup/be_thread.cc b/sql/backup/be_thread.cc
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/sql/backup/be_thread.cc	2007-11-07 16:48:05 -05:00
> @@ -0,0 +1,292 @@
> +/* Copyright (C) 2004-2007 MySQL AB
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
> +*/
> +
> +/**
> +  * @file
> +  *
> +  * @brief Contains the thread methods for online backup.
> +  *
> +  * The methods in this class are used to initialize the mutexes
> +  * for the backup threads. Helper methods are included to make thread
> +  * calls easier for the driver code.
> +  */
> +
> +#include "be_thread.h"
> +
> +/**
> +  *  @brief Creates a new THD object.
> +  *
> +  * Creates a new THD object for use in running as a separate thread.
> +  *
> +  * @returns Pointer to new THD object or 0 if error.
> +  *
> +  * @TODO Move this method to a location where ha_ndbcluster_binlog.cc can
> +  *       use it and replace code in ndb_binlog_thread_func(void *arg) to
> +  *       call this function.
> +  *
> +  * @note my_net_init() this should be paired with my_net_end() on 
> +  *       close/kill of thread.
> +  */
> +THD *create_new_thd()
> +{
> +  THD *thd;
> +  DBUG_ENTER("Create new THD object");
> +
> +  thd= new THD;
> +  if (unlikely(!thd))
> +  {
> +    delete thd;
> +    DBUG_RETURN(0);
> +  }
> +
> +  thd->thread_stack = (char*)&thd; // remember where our stack is  
> +  pthread_mutex_lock(&LOCK_thread_count);
> +  thd->thread_id= thread_id++;
> +  pthread_mutex_unlock(&LOCK_thread_count);
> +  if (unlikely(thd->store_globals())) // for a proper MEM_ROOT  
> +  {
> +    delete thd;
> +    DBUG_RETURN(0);
> +  }
> +
> +  thd->init_for_queries(); // opening tables needs a proper LEX
> +  thd->command= COM_DAEMON;
> +  thd->system_thread= SYSTEM_THREAD_BACKUP;
> +  thd->version= refresh_version;
> +  thd->set_time();
> +  thd->main_security_ctx.host_or_ip= "";
> +  thd->client_capabilities= 0;
> +  my_net_init(&thd->net, 0);
> +  thd->main_security_ctx.master_access= ~0;
> +  thd->main_security_ctx.priv_user= 0;
> +  thd->real_id= pthread_self();
> +  /*
> +    Making this thread visible to SHOW PROCESSLIST is useful for
> +    troubleshooting a backup job (why does it stall etc).
> +  */
> +  pthread_mutex_lock(&LOCK_thread_count);
> +  threads.append(thd);
> +  pthread_mutex_unlock(&LOCK_thread_count);
> +  lex_start(thd);
> +  mysql_reset_thd_for_next_command(thd);
> +  DBUG_RETURN(thd);
> +}
> +
> +/**
> +  * @brief Lock tables in driver.
> +  *
> +  * This method creates a new THD for use in the new thread. It calls
> +  * the method to open and lock the tables.
> +  *
> +  * @note my_thread_init() should be paired with my_thread_end() on 
> +  *       close/kill of thread.
> +  */
> +pthread_handler_t backup_thread_for_locking(void *arg)
> +{
> +  Backup_thread_driver *drv= static_cast<Backup_thread_driver *>(arg);
> +
> +  DBUG_PRINT("info", ("Default_backup - lock_tables_in_separate_thread"));
> +
> +  /*
> +    Turn off condition variable check for lock.
> +  */
> +  drv->lock_state= LOCK_NOT_STARTED;
> +
> +#if !defined( __WIN__) /* Win32 calls this in pthread_create */
> +  my_thread_init();
> +#endif
> +
> +  pthread_detach_this_thread();
> +
> +  /*
> +    First, create a new THD object.
> +  */
> +  DBUG_PRINT("info",("Online backup creating THD struct for thread"));
> +  THD *thd= create_new_thd();
> +  drv->lock_thd= thd;
> +  if (thd == 0)
> +  {
> +    drv->lock_state= LOCK_ERROR;
> +    goto end2;
> +  }
> +
> +  if (thd->killed)
> +  {
> +    drv->lock_state= LOCK_ERROR;

Q: Should we treat it as error?

> +    goto end2;
> +  }
> +
> +  /* 
> +    Now open and lock the tables.
> +  */
> +  DBUG_PRINT("info",("Online backup open tables in thread"));
> +  if (!drv->tables_in_backup)
> +  {
> +    DBUG_PRINT("info",("Online backup locking error no tables to lock"));
> +    drv->lock_state= LOCK_ERROR;

Q: Should we treat it as error?

> +    goto end2;
> +  }
> +
> +  /*
> +    As locking tables can be a long operation, we need to support
> +    killing the thread. In this case, we need to close the tables 
> +    and exit.
> +  */
> +  if (!thd->killed && open_and_lock_tables(thd,
> drv->tables_in_backup))
> +  {
> +    DBUG_PRINT("info",("Online backup locking thread dying"));
> +    drv->lock_state= LOCK_ERROR;
> +    goto end;
> +  }
> +
> +  if (thd->killed)
> +  {
> +    drv->lock_state= LOCK_ERROR;

Q: Should we treat it as error?

> +    goto end;
> +  }
> +
> +  drv->lock_state= LOCK_ACQUIRED;
> +
> +  /*
> +    Part of work is done. Rest until woken up.
> +    We wait if the thread is not killed and the driver has not signaled us.
> +  */
> +  pthread_mutex_lock(&drv->THR_LOCK_driver_thread);
> +  thd->enter_cond(&drv->COND_driver_thread_wait,
> &drv->THR_LOCK_driver_thread,
> +                  "Online backup driver thread: holding table locks");
> +  while (!thd->killed && (drv->lock_state != LOCK_SIGNAL))
> +    pthread_cond_wait(&drv->COND_driver_thread_wait,
> &drv->THR_LOCK_driver_thread);
> +  thd->exit_cond("Online backup driver thread: terminating");
> +
> +  DBUG_PRINT("info",("Online backup driver thread locking thread terminating"));
> +
> +  /*
> +    Cleanup and return.
> +  */
> +end:
> +  close_thread_tables(thd);
> +
> +end2:
> +  pthread_mutex_lock(&drv->THR_LOCK_driver);
> +  net_end(&thd->net);
> +  my_thread_end();
> +  delete thd;

For safety reasons, better to set lock_thd to NULL here (doesn't the destructor 
rely on this?).

> +  if (drv->lock_state != LOCK_ERROR)
> +    drv->lock_state= LOCK_DONE;
> +  pthread_mutex_unlock(&drv->THR_LOCK_driver);
> +
> +  /*
> +    Signal the driver thread that it's ok to proceed with destructor.
> +  */
> +  pthread_cond_broadcast(&drv->COND_driver_wait);
> +  pthread_exit(0);
> +  return (0);
> +}
> +
> +/*
> +  Constructor for backup_thread_driver class.
> +*/
> +Backup_thread_driver::Backup_thread_driver(const Table_list &tables):
> +                                           Backup_driver(tables)
> +{
> +  /*
> +    Initialize the thread mutex and cond variable.
> +  */
> +  pthread_mutex_init(&THR_LOCK_driver_thread, MY_MUTEX_INIT_FAST);
> +  pthread_cond_init(&COND_driver_thread_wait, NULL);
> +  pthread_mutex_init(&THR_LOCK_driver, MY_MUTEX_INIT_FAST);
> +  pthread_cond_init(&COND_driver_wait, NULL);
> +  lock_state= LOCK_NOT_STARTED;
> +  lock_thd= NULL; // set to 0 as precaution for get_data being called too soon
> +};
> +
> +/*
> +  Destructor for backup_thread_driver class.
> +*/
> +Backup_thread_driver::~Backup_thread_driver()
> +{
> +  /*
> +    If the locking thread is not finished, we need to wait until
> +    it is finished so that we can destroy the mutexes safely knowing
> +    the locking thread won't access them.
> +  */
> +  if (lock_state != LOCK_DONE)
> +  {
> +    pthread_mutex_lock(&THR_LOCK_driver);
> +    m_thd->enter_cond(&COND_driver_wait, &THR_LOCK_driver,
> +                    "Online backup driver: waiting until locking thread is done");
> +    while (lock_state != LOCK_DONE)
> +      pthread_cond_wait(&COND_driver_wait, &THR_LOCK_driver);
> +    m_thd->exit_cond("Online backup driver: terminating");
> +
> +    DBUG_PRINT("info",("Online backup driver's locking thread terminated"));
> +  }
> +
> +  /*
> +    Destroy the thread mutexes and cond variables.
> +  */
> +  pthread_mutex_destroy(&THR_LOCK_driver_thread);
> +  pthread_cond_destroy(&COND_driver_thread_wait);
> +  pthread_mutex_destroy(&THR_LOCK_driver);
> +  pthread_cond_destroy(&COND_driver_wait);
> +}
> +
> +/**
> +   Start the driver's lock thread.
> +
> +   Launches a separate thread ("locking thread") which will lock
> +   tables.
> + */
> +result_t Backup_thread_driver::start_locking_thread()
> +{
> +  DBUG_ENTER("Backup_thread_driver::start_locking_thread");
> +  pthread_t th;
> +  if (pthread_create(&th, &connection_attrib,
> +                     backup_thread_for_locking, this))
> +    SET_STATE_TO_ERROR_AND_DBUG_RETURN;
> +  DBUG_RETURN(backup::OK);
> +}
> +
> +/**
> +   Kill the driver's lock thread.
> +
> +   This method issues the awake and broadcast to kill the locking thread.
> +   A mutex is used to prevent the locking thread from deleting the THD
> +   structure until this operation is complete.
> + */
> +void Backup_thread_driver::kill_locking_thread()
> +{
> +  DBUG_ENTER("Backup_thread_driver::kill_locking_thread");
> +  if (lock_state == LOCK_ACQUIRED)
> +  {
> +    pthread_mutex_lock(&THR_LOCK_driver);
> +    if (lock_thd)
> +    {
> +      pthread_mutex_lock(&lock_thd->LOCK_delete);
> +      lock_thd->awake(THD::KILL_CONNECTION);
> +      pthread_mutex_unlock(&lock_thd->LOCK_delete);
> +      pthread_cond_broadcast(&COND_driver_thread_wait);
> +    }
> +    /*
> +      Set the lock state to LOCK_SIGNAL to indicate we have
> +      signaled the locking thread that it can terminate.
> +    */
> +    lock_state= LOCK_SIGNAL;
> +    pthread_mutex_unlock(&THR_LOCK_driver);
> +  }
> +  DBUG_VOID_RETURN;
> +}
> +


> diff -Nrup a/sql/backup/be_thread.h b/sql/backup/be_thread.h
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/sql/backup/be_thread.h	2007-11-07 16:48:05 -05:00
> @@ -0,0 +1,79 @@
> +#ifndef _BACKUP_THREAD_H
> +#define _BACKUP_THREAD_H
> +
> +#include "../mysql_priv.h"
> +#include "archive.h"
> +#include "api_types.h"
> +#include "backup_engine.h"
> +
> +/**
> +   Macro for error handling.
> +*/
> +#define SET_STATE_TO_ERROR_AND_DBUG_RETURN {                                 \
> +    int state= backup::ERROR;                                                \
> +    DBUG_PRINT("error",("driver got an error at %s:%d",__FILE__,__LINE__)); \
> +    DBUG_RETURN(backup::ERROR); }
> +
> +/**
> +   Locking of tables goes through several states.
> +*/
> +typedef enum {
> +  LOCK_NOT_STARTED,
> +  LOCK_IN_PROGRESS,
> +  LOCK_ACQUIRED,
> +  LOCK_DONE,
> +  LOCK_ERROR,
> +  LOCK_SIGNAL
> +} LOCK_STATE;
> +
> +using backup::result_t;
> +using backup::Table_list;
> +
> +/**
> +   create_new_thd
> +
> +   This method creates a new THD object.
> +*/
> +THD *create_new_thd();
> +
> +/**
> +   backup_thread_for_locking
> +
> +   This method creates a new thread and opens and locks the tables.
> +*/
> +pthread_handler_t backup_thread_for_locking(void *arg);
> +
> +/**
> + * @class Backup_thread_driver
> + *
> + * @brief Adds variables for using a locking thread for opening tables.
> + *
> + * The Backup_thread_driver class extends the Backup_driver class by adding
> + * a mutex and condition variable for using a thread to open and lock the 
> + * tables.
> + *
> + * @see <backup driver> and <backup thread driver>
> + */
> +class Backup_thread_driver : public Backup_driver
> +{
> +public:
> +
> +  Backup_thread_driver(const backup::Table_list &tables);
> +  ~Backup_thread_driver();
> +
> +  pthread_mutex_t THR_LOCK_driver_thread; ///< mutex for thread variables
> +  pthread_cond_t COND_driver_thread_wait; ///< condition variable for wait
> +  pthread_mutex_t THR_LOCK_driver; ///< mutex for thread variables
> +  pthread_cond_t COND_driver_wait; ///< condition variable for wait
> +  TABLE_LIST *tables_in_backup;           ///< List of tables used in backup
> +  THD *lock_thd;                          ///< Locking thread pointer
> +  LOCK_STATE lock_state;                  ///< Current state of the lock call
> +  THD *m_thd;                    ///< Pointer to current thread struct.
> +
> +  backup::result_t start_locking_thread();
> +  void kill_locking_thread();
> +
> +}; // Backup_thread_driver class
> +
> +#endif
> +


> diff -Nrup a/sql/backup/data_backup.cc b/sql/backup/data_backup.cc
> --- a/sql/backup/data_backup.cc	2007-09-11 05:29:29 -04:00
> +++ b/sql/backup/data_backup.cc	2007-11-07 16:48:03 -05:00
> @@ -384,9 +384,6 @@ int write_table_data(THD*, Backup_info &
>  
>    DBUG_PRINT("backup/data",("initializing scheduler"));
>  
> -  TABLE_LIST *table_list= 0;
> -  TABLE_LIST *table_list_last= 0;
> -
>    // add unknown "at end" drivers to scheduler, rest to inactive list
>  
>    for (uint no=0; no < info.img_count; ++no)
> @@ -418,12 +415,6 @@ int write_table_data(THD*, Backup_info &
>  
>        inactive.push_back(p);
>      }
> -    if (!def_or_snap_used)
> -      def_or_snap_used=  ((i->type() == Image_info::DEFAULT_IMAGE) ||
> -                          (i->type() == Image_info::SNAPSHOT_IMAGE));
> -    if (def_or_snap_used)
> -      get_default_snapshot_tables(&p->drv(), NULL, 
> -                                  &table_list, &table_list_last);
>    }
>  
>    /*
> @@ -508,22 +499,6 @@ int write_table_data(THD*, Backup_info &
>      if (sch.prepare())
>        goto error;
>  
> -    /*
> -      Open tables for default and snapshot drivers.
> -    */
> -    if (table_list)
> -    {
> -      if (open_and_lock_tables(::current_thd, table_list))
> -      {
> -        DBUG_PRINT("backup", 
> -          ( "error on open tables for default and snapshot drivers!" ));
> -        info.report_error(ER_BACKUP_OPEN_TABLES, "backup");
> -        DBUG_RETURN(ERROR);
> -      }
> -      if (table_list_last)
> -        table_list_last->next_global= NULL; // break lists
> -    }
> -
>      while (sch.prepare_count > 0)
>      if (sch.step())
>        goto error;
> @@ -548,12 +523,6 @@ int write_table_data(THD*, Backup_info &
>  
>      DBUG_PRINT("backup/data",("-- DONE --"));
>    }
> -
> -  /*
> -    If the default or snapshot drivers are used, close the tables.
> -  */
> -  if (def_or_snap_used)
> -    close_thread_tables(::current_thd);
>  
>    info.data_size= s.bytes - start_bytes;
>  


> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt	2007-10-17 09:57:35 -04:00
> +++ b/sql/share/errmsg.txt	2007-11-07 16:48:04 -05:00
> @@ -6195,6 +6195,9 @@ ER_BACKUP_SEND_DATA_RETRY
>  ER_BACKUP_OPEN_TABLES
>          eng "Open and lock tables failed in %-.64s"
>  
> +ER_BACKUP_THREAD_INIT
> +        eng "Backup driver's table locking thread can not be initialized."
> +
>  ER_VIEW_NO_CREATION_CTX
>    eng "View `%-.64s`.`%-.64s` has no creation context"
>  ER_VIEW_INVALID_CREATION_CTX


> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h	2007-09-13 04:56:30 -04:00
> +++ b/sql/sql_class.h	2007-11-07 16:47:58 -05:00
> @@ -936,7 +936,8 @@ enum enum_thread_type
>    SYSTEM_THREAD_SLAVE_SQL= 4,
>    SYSTEM_THREAD_NDBCLUSTER_BINLOG= 8,
>    SYSTEM_THREAD_EVENT_SCHEDULER= 16,
> -  SYSTEM_THREAD_EVENT_WORKER= 32
> +  SYSTEM_THREAD_EVENT_WORKER= 32,
> +  SYSTEM_THREAD_BACKUP= 64
>  };