Hi Rafal!
Thanks for the review. Comments on your comments below.
Just my few cents,
Mats Kindahl
Rafal Somla wrote:
> Hi Mats,
>
> Excellent comments. On that occasion I propose to add documentation
> for terminate_slave_thread() and clean-up it bit more by removing two
> sepatate mutexes (term_lock and cond_lock) as in my opinion it is not
> going to work anyway and is potentially dangerous. See below for
> details of my proposition.
>
> Mats Kindahl wrote:
>> Below is the list of changes that have just been committed into a local
>> 5.1 repository of mats. When mats 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-08-21 16:00:21+02:00,
>> mats@stripped +2 -0
>> BUG#29968 (rpl_ndb_circular.test and rpl_ndb_log.test fail):
>> There appears to be a bug in some implementations of
>> pthread_cond_timedwait() in that it does not lock the associated mutex
>> when returning, so this patch attempts a workaround to acquire the
>> mutex
>> even in that case to prevent the mutex from being destroyed while
>> still
>> being locked by the the slave thread.
>>
>> sql/slave.cc@stripped, 2007-08-21 16:00:09+02:00,
>> mats@stripped +90 -5
>> Making terminate_slave_thread() static since it is not used
>> elsewhere.
>> Adding code to handle buggy implementation of
>> pthread_cond_timedwait().
>>
>> sql/slave.h@stripped, 2007-08-21 16:00:09+02:00,
>> mats@stripped +0 -4
>> Moving terminate_slave_thread() to use internal linkage.
>>
>> diff -Nrup a/sql/slave.cc b/sql/slave.cc
>> --- a/sql/slave.cc 2007-07-27 02:26:39 +02:00
>> +++ b/sql/slave.cc 2007-08-21 16:00:09 +02:00
>> @@ -132,6 +132,11 @@ static int create_table_from_dump(THD* t
>> const char* table_name, bool
>> overwrite);
>> static int get_master_version_and_clock(MYSQL* mysql, MASTER_INFO* mi);
>> static Log_event* next_event(RELAY_LOG_INFO* rli);
>> +static int terminate_slave_thread(THD* thd,
>> + pthread_mutex_t* term_lock,
>> + pthread_mutex_t *cond_lock,
>> + pthread_cond_t* term_cond,
>> + volatile uint *slave_running);
>>
>> /*
>> Find out which replications threads are running
>> @@ -348,11 +353,14 @@ int terminate_slave_threads(MASTER_INFO*
>> }
>>
>>
>> -int terminate_slave_thread(THD* thd, pthread_mutex_t* term_lock,
>> - pthread_mutex_t *cond_lock,
>> - pthread_cond_t* term_cond,
>> - volatile uint *slave_running)
>> +static int
>> +terminate_slave_thread(THD* thd,
>> + pthread_mutex_t* term_lock,
>> + pthread_mutex_t *cond_lock, pthread_cond_t*
>> term_cond,
>> + volatile uint *slave_running)
>
> I propose to add documentation for this function. My proposition (feel
> free to rephrase it better):
>
> /**
> Wait for slave I/O or SQL thread to terminate.
>
> This function should be called after requesting the thread to
> terminate (by
> setting abort_slave member of RLI or MI structure to 1). The fact
> that thread
> has terminated is detected by checking flag pointed by @c
> slave_running. This
> flag should be guarded by @c term_lock to avoid race conditions and
> its change
> schould be signaled by broadcasting @c term_cond.
>
> In case of I/O thread:
> @code
> term_lock = mi->run_lock,
> term_cond = mi->stop_cond,
> slave_running = &mi->slave_runnig;
> @endcode
> in case of SQL thread:
> @code
> term_lock = mi->rli.run_lock,
> term_cond = mi->rli.stop_cond,
> slave_running = &mi->rli.slave_runnig;
> @endcode
> where @c mi is the master info structure.
>
> Function periodically wakes-up slave threads to ensure that they make
> progress. This is done using thd->awake() method (in KICK_SLAVE macro).
>
> @returns ...
> */
>
> int terminate_slave_thread(THD* thd,
> pthread_mutex_t *const term_lock,
> pthread_cond_t *const term_cond,
> volatile bool *const slave_running)
>
>> {
>> + int error;
>> +
>> DBUG_ENTER("terminate_slave_thread");
>> if (term_lock)
>> {
>
> I removed two separate mutex pointers from function's signature as the
> code
> can not work correctly if two different mutexes are used or no mutex
> is locked.
The precondition for the term_lock and cond_lock are
term_lock == NULL || term_lock == cond_lock
In other words, if term_lock is passed as NULL, no lock is acquired but
the cond_lock is needed to execute pthread_cond_timedwait(). See how
skip_lock is used in terminate_slave_threads() and see where
terminate_slave_threads() are called with skip_lock == true. In other
words, term_lock should not necessarily be locked inside the function.
> The initial code of the function would have to be changed to something
> like this:
>
> {
> int error;
>
> DBUG_ENTER("terminate_slave_thread");
> DBUG_ASSERT(thd && term_lock && term_cond &&
> slave_running)
>
> pthread_mutex_lock(term_lock);
> if (!*slave_running)
> {
> pthread_mutex_unlock(term_lock);
> DBUG_RETURN(ER_SLAVE_NOT_RUNNING);
> }
> THD_CHECK_SENTRY(thd);
> ...
>
>> @@ -380,8 +388,85 @@ int terminate_slave_thread(THD* thd, pth
>> */
>> struct timespec abstime;
>> set_timespec(abstime,2);
>> - pthread_cond_timedwait(term_cond, cond_lock, &abstime);
>> + error= pthread_cond_timedwait(term_cond, cond_lock, &abstime);
>> + DBUG_ASSERT(error == ETIMEOUT || error == 0);
>> + }
>> +
>> + DBUG_ASSERT(*slave_running == 0);
>> +
>> + /*
>> + BUG#29968: At this point, cond_lock should be held since
>> + pthread_cond_timedwait() always acquire the associated mutex even
>> + in the case of an error. However, it appears to be a bug in some
>> + thread libraries in that the function can return without acquiring
>> + the mutex (see http://bugs.mysql.com/bug.php?id=29968 for more
>> + info), so we have to take some precautions to ensure that the
>> + slave thread really have terminated (i.e., executed the
>> + pthread_mutex_unlock(cond_lock) statement) and we own the
>> + cond_lock mutex.
>> +
>> + We are assuming that the basic operations on mutexes work as they
>> + should, but that there is something with the handling of
>> + conditions that causes the premature return with the mutex
>> + unlocked. If not: there is not really much we can do except advice
>> + use of a working POSIX threads library.
>> +
>> + If we could just lock and unlock the cond_lock, we would be home
>> + free, but since calling pthread_mutex_lock() when we already own
>> + the mutex might cause a deadlock, we cannot do this and instead
>> + have to resort to the method below.
>> +
>> + Since *slave_running == 0, we know that the slave thread is
>> + executing somewhere after it being set to 0 and the end of the
>> + function (either handle_slave_io or handle_slave_sql). The only
>> + things that the slave thread does, is signalling some conditions
>> + and unlocking some mutexes, i.e., nothing that can block the
>> + thread and prevent it from terminating.
>> +
>> + To see if we have the mutex or not, we try to lock it.
>> + */
>> + error= pthread_mutex_trylock(cond_lock);
>> +
>> + /*
>> + We now have three cases to consider:
>> + 0 The mutex was not locked and we now own it
>> + EBUSY The mutex was locked either by us or by the slave thread
>> +
>> + If we got 0, there is nothing more to do, otherwise, we have to
>> + decide if we own the mutex and the slave thread has terminated, or
>> + if the slave thread is still executing and we need to wait for it
>
> "... if the slave thread is still executing, owning the mutex, and we
> ..."
Good point.
>
>> + to terminate.
>> + */
>> + DBUG_ASSERT(error == 0 || error == EBUSY);
>> + if (error == EBUSY)
>> + {
>> + /*
>> + Here we know that the mutex was locked either by us or by the
>> + slave thread. By unlocking it, we can check if we did own it. In
>> + the event that the mutex is released by the slave thread, or the
>> + slave thread owns it, this call is harmless (and will return the
>> + same error in both cases).
>> + */
>> + error= pthread_mutex_unlock(cond_lock);
>> +
>> + /*
>> + We now have two cases:
>> + 0 We owned the mutex and have now released it
>> + EPERM We didn't own the mutex (and doesn't after the call either)
>
> "(and don't ...)"
Right.
>
>> +
>> + In either case, we do not own the mutex right now, so it is safe
>> + to call pthread_mutex_lock() to acquire the mutex.
>> + */
>> + DBUG_ASSERT(error == 0 || error == EPERM);
>> + pthread_mutex_lock(cond_lock);
>> }
>> +
>> + /*
>> + Here, we own the cond_lock mutex and am not relying on
> "are not ..."
Right, I'm using "we" not "I"... :)
>> + pthread_cond_timedwait() to work correctly, we are just relying on
>> + pthread_mutex_{lock,trylock,unlock} to work correctly.
>> + */
>> +
>> if (term_lock)
>> pthread_mutex_unlock(term_lock);
>> DBUG_RETURN(0);
>> diff -Nrup a/sql/slave.h b/sql/slave.h
>> --- a/sql/slave.h 2007-06-09 08:29:44 +02:00
>> +++ b/sql/slave.h 2007-08-21 16:00:09 +02:00
>> @@ -133,10 +133,6 @@ bool flush_relay_log_info(RELAY_LOG_INFO
>> int register_slave_on_master(MYSQL* mysql);
>> int terminate_slave_threads(MASTER_INFO* mi, int thread_mask,
>> bool skip_lock = 0);
>> -int terminate_slave_thread(THD* thd, pthread_mutex_t* term_mutex,
>> - pthread_mutex_t* cond_lock,
>> - pthread_cond_t* term_cond,
>> - volatile uint* slave_running);
>> int start_slave_threads(bool need_slave_mutex, bool wait_for_start,
>> MASTER_INFO* mi, const char* master_info_fname,
>> const char* slave_info_fname, int thread_mask);
>>
>
> Rafal
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
