Chuck,
I saw slightly different code for registering a new thread in event_scheduler.cc
(see below). Please check that your code is doing all what is supposed to be
done. See also my other comments below.
Rafal
cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 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-12-19 10:20:33-05:00, cbell@mysql_cab_desk. +6 -0
> BUG#32970 : Locking thread not added to process list properly
>
> Solves the problem of the locking thread not being present in
> the processlist. This was due to the thd->query not being
> populated and the info attribute not supplied.
>
> mysql-test/r/backup.result@stripped, 2007-12-19 10:20:24-05:00, cbell@mysql_cab_desk.
> +22 -0
> BUG#32970 : Locking thread not added to process list properly
>
> New result file
>
> mysql-test/t/backup.test@stripped, 2007-12-19 10:20:24-05:00, cbell@mysql_cab_desk. +37
> -1
> BUG#32970 : Locking thread not added to process list properly
>
> Additional test to ensure locking thread is added to processlist.
>
> sql/backup/backup_progress.cc@stripped, 2007-12-19 10:20:25-05:00, cbell@mysql_cab_desk.
> +1 -0
> BUG#32970 : Locking thread not added to process list properly
>
> Add identification of locking thread.
>
> sql/backup/be_default.cc@stripped, 2007-12-19 10:20:25-05:00, cbell@mysql_cab_desk. +2
> -0
> BUG#32970 : Locking thread not added to process list properly
>
> Add identification of locking thread and insert breakpoint for
> new test.
>
> sql/backup/be_thread.cc@stripped, 2007-12-19 10:20:26-05:00, cbell@mysql_cab_desk. +26
> -9
> BUG#32970 : Locking thread not added to process list properly
>
> Improved thread (THD) structure behavior and add it to the list
> of threads. Inserts info update macro calls to update the
> state of the thread.
>
> sql/backup/be_thread.h@stripped, 2007-12-19 10:20:27-05:00, cbell@mysql_cab_desk. +1 -7
> BUG#32970 : Locking thread not added to process list properly
>
> Hide create_new_thd() method. Add name attribute to thread
> structure.
>
> diff -Nrup a/mysql-test/r/backup.result b/mysql-test/r/backup.result
> --- a/mysql-test/r/backup.result 2007-12-13 20:58:12 -05:00
> +++ b/mysql-test/r/backup.result 2007-12-19 10:20:24 -05:00
> @@ -1,3 +1,4 @@
> +SET GLOBAL debug="d,backup_debug:d,backup";
> DROP DATABASE IF EXISTS db1;
> DROP DATABASE IF EXISTS db2;
> DROP DATABASE IF EXISTS db3;
> @@ -41,7 +42,28 @@ CREATE TABLE `tasking` (
> LOCK TABLES `tasking` WRITE;
> INSERT INTO `tasking` VALUES
> ('333445555','405',23),('123763153','405',33.5),('921312388','601',44),('800122337','300',13),('820123637','300',9.5),('830132335','401',8.5),('333445555','300',11),('921312388','500',13),('800122337','300',44),('820123637','401',500.5),('830132335','400',12),('333445665','600',300.25),('123654321','607',444.75),('123456789','300',1000);
> UNLOCK TABLES;
> +breakpoints: Getting lock "locking_thread_added"
> +SELECT get_lock("locking_thread_added", 100);
> +get_lock("locking_thread_added", 100)
> +1
> +backup: Send the backup command.
> BACKUP DATABASE db1,db2 TO 'test.ba';
> +breakpoints: Check for lock in process list.
> +breakpoints: Checking locks.
> +SELECT id, command, state, info FROM INFORMATION_SCHEMA.PROCESSLIST
> +WHERE info LIKE "BACKUP DATABASE%" OR info LIKE "default driver locking thread%";
> +id #
> +command Daemon
> +state Locking thread: holding table locks
> +info default driver locking thread
> +id #
> +command Query
> +state debug_sync_point: locking_thread_added
> +info BACKUP DATABASE db1,db2 TO 'test.ba'
> +breakpoints: Release lock
> +SELECT release_lock("locking_thread_added");
> +release_lock("locking_thread_added")
> +1
> backup_id
> #
> DROP DATABASE db1;
> diff -Nrup a/mysql-test/t/backup.test b/mysql-test/t/backup.test
> --- a/mysql-test/t/backup.test 2007-12-14 02:51:28 -05:00
> +++ b/mysql-test/t/backup.test 2007-12-19 10:20:24 -05:00
Shouldn't a similar test be added to backup_progress?
> @@ -1,10 +1,15 @@
> --source include/have_innodb.inc
> --source include/not_embedded.inc
> +--source include/have_debug.inc
>
> connect (backup,localhost,root,,);
> +connect (breakpoints,localhost,root,,);
>
> connection backup;
>
> +# Setup the server to use the backup breakpoints
> +SET GLOBAL debug="d,backup_debug:d,backup";
> +
> --disable_warnings
> DROP DATABASE IF EXISTS db1;
> DROP DATABASE IF EXISTS db2;
> @@ -93,9 +98,39 @@ LOCK TABLES `tasking` WRITE;
> INSERT INTO `tasking` VALUES
> ('333445555','405',23),('123763153','405',33.5),('921312388','601',44),('800122337','300',13),('820123637','300',9.5),('830132335','401',8.5),('333445555','300',11),('921312388','500',13),('800122337','300',44),('820123637','401',500.5),('830132335','400',12),('333445665','600',300.25),('123654321','607',444.75),('123456789','300',1000);
> UNLOCK TABLES;
>
> +#
> +# Get a lock to check for locking thread in process list
> +#
> +connection breakpoints;
> +--echo breakpoints: Getting lock "locking_thread_added"
> +SELECT get_lock("locking_thread_added", 100);
> +
> +connection backup;
> +--echo backup: Send the backup command.
> +send BACKUP DATABASE db1,db2 TO 'test.ba';
> +
> +# get the lock and wait until lock is identified in process list
> +connection breakpoints;
> +--echo breakpoints: Check for lock in process list.
> +
> +# Wait for lock to be acquired and execution to reach breakpoint
> +--echo breakpoints: Checking locks.
> +let $wait_condition = SELECT count(*) = 1
> + FROM INFORMATION_SCHEMA.PROCESSLIST
> + WHERE info LIKE "default driver locking thread%";
> +--source include/wait_condition.inc
> +
> +--replace_column 1 #
> +query_vertical SELECT id, command, state, info FROM INFORMATION_SCHEMA.PROCESSLIST
> +WHERE info LIKE "BACKUP DATABASE%" OR info LIKE "default driver locking thread%";
>
> +--echo breakpoints: Release lock
> +SELECT release_lock("locking_thread_added");
> +
> +# reattach to backup job
> +connection backup;
> --replace_column 1 #
> -BACKUP DATABASE db1,db2 TO 'test.ba';
> +reap;
>
> DROP DATABASE db1;
> DROP DATABASE db2;
> @@ -293,4 +328,5 @@ DROP DATABASE IF EXISTS bup_default;
> --enable_warnings
>
> --remove_file $MYSQLTEST_VARDIR/master-data/bup_default.bak
> +--remove_file $MYSQLTEST_VARDIR/master-data/test.ba
>
> diff -Nrup a/sql/backup/backup_progress.cc b/sql/backup/backup_progress.cc
> --- a/sql/backup/backup_progress.cc 2007-12-06 13:04:38 -05:00
> +++ b/sql/backup/backup_progress.cc 2007-12-19 10:20:25 -05:00
> @@ -70,6 +70,7 @@ Locking_thread_st *open_backup_progress_
> /*
> Start the locking thread and wait until it is ready.
> */
> + locking_thd->thd_name.append("backup progress locking thread");
> locking_thd->start_locking_thread();
>
> /*
> diff -Nrup a/sql/backup/be_default.cc b/sql/backup/be_default.cc
> --- a/sql/backup/be_default.cc 2007-12-13 09:08:41 -05:00
> +++ b/sql/backup/be_default.cc 2007-12-19 10:20:25 -05:00
> @@ -138,6 +138,7 @@ Backup::Backup(const Table_list &tables,
> result_t Backup::prelock()
> {
> DBUG_ENTER("Default_backup::prelock()");
> + locking_thd->thd_name.append("default driver locking thread");
Ouch! why locking thread's name is not set inside its constructor? It looks
really bad if it is set externally by the user of the class like this. If it is
possible please change this.
> DBUG_RETURN(locking_thd->start_locking_thread());
> }
>
> @@ -297,6 +298,7 @@ result_t Backup::get_data(Buffer &buf)
> case LOCK_ACQUIRED: // First time lock ready for validity point
> {
> locks_acquired= TRUE;
> + BACKUP_BREAKPOINT("locking_thread_added");
> DBUG_RETURN(READY);
> }
> default: // If first call, signal end of init phase
> diff -Nrup a/sql/backup/be_thread.cc b/sql/backup/be_thread.cc
> --- a/sql/backup/be_thread.cc 2007-12-04 12:38:06 -05:00
> +++ b/sql/backup/be_thread.cc 2007-12-19 10:20:26 -05:00
> @@ -37,13 +37,6 @@
> * use it and replace code in ndb_binlog_thread_func(void *arg) to
> * call this function.
> *
> - * @TODO Make this thread visible to SHOW PROCESSLIST. The following code
> - * can be used to do this. See BUG#32970 for more details.
> - *
> - * pthread_mutex_lock(&LOCK_thread_count);
> - * threads.append(thd);
> - * pthread_mutex_unlock(&LOCK_thread_count);
> - *
> * @note my_net_init() this should be paired with my_net_end() on
> * close/kill of thread.
> */
> @@ -58,6 +51,7 @@ THD *create_new_thd()
> delete thd;
> DBUG_RETURN(0);
> }
> + THD_CHECK_SENTRY(thd);
>
> thd->thread_stack = (char*)&thd; // remember where our stack is
> pthread_mutex_lock(&LOCK_thread_count);
> @@ -65,6 +59,7 @@ THD *create_new_thd()
> pthread_mutex_unlock(&LOCK_thread_count);
> if (unlikely(thd->store_globals())) // for a proper MEM_ROOT
> {
> + thd->cleanup();
> delete thd;
> DBUG_RETURN(0);
> }
> @@ -83,6 +78,15 @@ THD *create_new_thd()
>
> lex_start(thd);
> mysql_reset_thd_for_next_command(thd);
> +
> + /*
> + 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);
> +
In event_scheduler.cc I found similar code for initializing a new thread:
> pthread_mutex_lock(&LOCK_thread_count);
> threads.append(thd);
> thread_count++;
> thread_running++;
> pthread_mutex_unlock(&LOCK_thread_count);
I wonder if it is correct to skip the counters in your code?
> DBUG_RETURN(thd);
> }
>
> @@ -110,22 +114,28 @@ pthread_handler_t backup_thread_for_lock
> 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();
> +
> + THD_SET_PROC_INFO(thd, "Locking thread started");
> + thd->query= locking_thd->thd_name.c_ptr();
> + thd->query_length= locking_thd->thd_name.length();
> +
> + pthread_detach_this_thread();
> locking_thd->lock_thd= thd;
> if (thd == 0)
> {
> + THD_SET_PROC_INFO(thd, "lock error");
> locking_thd->lock_state= LOCK_ERROR;
> goto end2;
> }
>
> if (thd->killed)
> {
> + THD_SET_PROC_INFO(thd, "lock error");
> locking_thd->lock_state= LOCK_ERROR;
> goto end2;
> }
> @@ -137,6 +147,7 @@ pthread_handler_t backup_thread_for_lock
> if (!locking_thd->tables_in_backup)
> {
> DBUG_PRINT("info",("Online backup locking error no tables to lock"));
> + THD_SET_PROC_INFO(thd, "lock error");
> locking_thd->lock_state= LOCK_ERROR;
> goto end2;
> }
> @@ -149,12 +160,14 @@ pthread_handler_t backup_thread_for_lock
> if (!thd->killed && open_and_lock_tables(thd,
> locking_thd->tables_in_backup))
> {
> DBUG_PRINT("info",("Online backup locking thread dying"));
> + THD_SET_PROC_INFO(thd, "lock error");
> locking_thd->lock_state= LOCK_ERROR;
> goto end;
> }
>
> if (thd->killed)
> {
> + THD_SET_PROC_INFO(thd, "lock error");
> locking_thd->lock_state= LOCK_ERROR;
> goto end;
> }
> @@ -163,6 +176,7 @@ pthread_handler_t backup_thread_for_lock
> Part of work is done. Rest until woken up.
> We wait if the thread is not killed and the driver has not signaled us.
> */
> + THD_SET_PROC_INFO(thd, "waiting for signal");
> pthread_mutex_lock(&locking_thd->THR_LOCK_thread);
> locking_thd->lock_state= LOCK_ACQUIRED;
> thd->enter_cond(&locking_thd->COND_thread_wait,
> @@ -172,6 +186,7 @@ pthread_handler_t backup_thread_for_lock
> pthread_cond_wait(&locking_thd->COND_thread_wait,
> &locking_thd->THR_LOCK_thread);
> thd->exit_cond("Locking thread: terminating");
> + THD_SET_PROC_INFO(thd, "terminating");
>
> DBUG_PRINT("info",("Locking thread locking thread terminating"));
>
> @@ -189,6 +204,7 @@ end2:
> locking_thd->lock_thd= NULL;
> if (locking_thd->lock_state != LOCK_ERROR)
> locking_thd->lock_state= LOCK_DONE;
> + THD_SET_PROC_INFO(thd, "lock done");
>
> /*
> Signal the driver thread that it's ok to proceed with destructor.
> @@ -213,6 +229,7 @@ Locking_thread_st::Locking_thread_st()
> pthread_cond_init(&COND_caller_wait, NULL);
> lock_state= LOCK_NOT_STARTED;
> lock_thd= NULL; // set to 0 as precaution for get_data being called too soon
> + thd_name.length(0);
> };
>
> /*
> diff -Nrup a/sql/backup/be_thread.h b/sql/backup/be_thread.h
> --- a/sql/backup/be_thread.h 2007-11-30 03:28:56 -05:00
> +++ b/sql/backup/be_thread.h 2007-12-19 10:20:27 -05:00
> @@ -29,13 +29,6 @@ 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.
> @@ -66,6 +59,7 @@ public:
> THD *lock_thd; ///< Locking thread pointer
> LOCK_STATE lock_state; ///< Current state of the lock call
> THD *m_thd; ///< Pointer to current thread struct.
> + String thd_name; ///< Name of locking thread
>
> result_t start_locking_thread();
> void kill_locking_thread();
>
>
