Re: bk commit into 5.0 tree (sven:1.2589) BUG#31024

[Mats Kindahl]

Hi Sven!

Comments on patch inline.

Just my few cents,
Mats Kindahl

Sven Sandberg wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of sven.  When sven 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, 2008-03-11 10:03:23+01:00, sven@riska.(none) +2 -0
>   BUG#31024: STOP SLAVE does not stop attempted connect()s
>   Problem: if the IO slave thread is attempting to connect,
>   STOP SLAVE waits for the attempt to finish. 
>   It may take a long time.
>   Fix: don't wait, stop the slave immediately.
>
>   sql/slave.cc@stripped, 2008-03-11 10:03:21+01:00, sven@riska.(none) +17 -1
>     Send a SIGALRM signal to the slave thread when stopping it (using
>     pthread_kill()). This breaks current socket(), connect(), poll() etc.
>     calls, and makes the subsequent thd->awake() call effective.
>     
>     Also, move the definition of KICK_SLAVE to slave.cc.
>
>   sql/sql_repl.h@stripped, 2008-03-11 10:03:21+01:00, sven@riska.(none) +0 -6
>     Removed KICK_SLAVE and inlined it in slave.cc because:
>      - it was only called once, so better to make it local to where it is used
>      - it needed to include a preprocessor conditional in the middle
>
> diff -Nrup a/sql/slave.cc b/sql/slave.cc
> --- a/sql/slave.cc	2008-02-14 12:55:09 +01:00
> +++ b/sql/slave.cc	2008-03-11 10:03:21 +01:00
> @@ -699,7 +699,23 @@ int terminate_slave_thread(THD* thd, pth
>    while (*slave_running)			// Should always be true
>    {
>      DBUG_PRINT("loop", ("killing slave thread"));
> -    KICK_SLAVE(thd);
> +
> +    pthread_mutex_lock(&thd->LOCK_delete);
> +#ifndef DONT_USE_THR_ALARM
> +    /*
> +      Error codes from pthread_kill are:
> +      EINVAL: invalid signal number (can't happen)
> +      ESRCH: thread already killed (can happen, should be ignored)
> +    */
> +#ifdef DEBUG
> +    DBUG_ASSERT(pthread_kill(thd->real_id, thr_client_alarm) != EINVAL);
> +#else
> +    pthread_kill(thd->real_id, thr_client_alarm);
> +#endif
> +#endif
>   

Now, this might seem silly, but I would really prefer if you could use 
the same function call and just assert the return value. The reason is 
that if the parameters are changed in one of the functions, but not the 
other, we will have a difference in the two execution branches. This 
might seem like a small thing, but these items can actually take hours 
to find, also, they might cause failed compiles in pushbuild, since one 
of the branches are not executed during regular (debug) build.

Something like the following:

while (*slave_running) {
  IF_DBUG(int error;)
  ...
  IF_DBUG(error= ) pthread_kill(thd->real_id, thr_client_alarm);
  DBUG_ASSERT(error != EINVAL);
  ...
}

> +    thd->awake(THD::NOT_KILLED);
> +    pthread_mutex_unlock(&thd->LOCK_delete);
> +
>      /*
>        There is a small chance that slave thread might miss the first
>        alarm. To protect againts it, resend the signal until it reacts
> diff -Nrup a/sql/sql_repl.h b/sql/sql_repl.h
> --- a/sql/sql_repl.h	2006-12-30 21:02:07 +01:00
> +++ b/sql/sql_repl.h	2008-03-11 10:03:21 +01:00
> @@ -35,12 +35,6 @@ extern I_List<i_string> binlog_do_db, bi
>  extern int max_binlog_dump_events;
>  extern my_bool opt_sporadic_binlog_dump_fail;
>  
> -#define KICK_SLAVE(thd) do {                                            \
> -                          pthread_mutex_lock(&(thd)->LOCK_delete);      \
> -                          (thd)->awake(THD::NOT_KILLED);                \
> -                          pthread_mutex_unlock(&(thd)->LOCK_delete);    \
> -                        } while(0)
> -
>  int start_slave(THD* thd, MASTER_INFO* mi, bool net_report);
>  int stop_slave(THD* thd, MASTER_INFO* mi, bool net_report);
>  bool change_master(THD* thd, MASTER_INFO* mi);
>
>   


-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com