Re: bzr commit into mysql-6.0-backup branch (cbell:2737) Bug#40434

[Rafal Somla]

STATUS
------
Patch not approved - changes requested.

REQUESTS
--------
1. Implement this functionality inside Backup_restore_ctx - see [1] below.

2. Remove the possibility of race condition described in [4].

SUGGESTIONS
-----------
1. Initialize allow_slave_start in init_slave_start() [2].

2. Move the code used to implement this functionality from rpl_failsafe to 
sql_rpl. The latter contains code related to starting replication on the server, 
the former code related to failover handling - your code does not belong here.

3. Instead of introducing new restore_running(), use is_slave() function for the 
purpose after changing its semantics as follows:

/**
   Check if current server is executing as a replication slave and possibly
   prevent it from becoming one.

   @param[IN]	block	if the server is not a slave, should it be blocked from
                         becoming one?

   This method can be used to detect when running as a slave. This means that
   the server is configured to act as a slave and the replication is running.

   In case the server is not a slave at the moment and block is TRUE, the
   possibility of starting replication on this server will be blocked until the
   block is removed with another call to @c is_slave(FALSE).

   If the server is not a slave and block is FALSE, then any blocks introduced by
   earlier calls to @c is_slave(TRUE) will be removed. It is ok to call
   is_slave(FALSE) even if no blocks were introduced earlier. Note that if the
   server is a replication slave, then obviously there are no blocks in effect.

   This can make it easier to
   organize the code for specialized sections where running as a slave requires
   additional work, prohibiting backup and restore operations, or error
   conditions.

   @returns   TRUE  if operating as a slave
*/
bool is_slave(bool block);

4. If you don't follow suggestion 3, consider renaming restore_running() for the 
reasons indicated in [3] below.R: implement inside Backup_restore_ctx

DETAILS
-------
Chuck Bell wrote:
> #At file:///C:/source/bzr/mysql-6.0-bug-40434/ based on
> revid:rafal.somla@stripped
> 
>  2737 Chuck Bell	2008-12-02
>       BUG#40434 : Replication should not be allowed to start if restore is running
>       
>       Currently, the work of WL#4209 and WL#4280 provide mechanisms to control 
>       how replication and backup interact. However, we also need to have a method 
>       to prevent replication from starting on a slave if a restore is ongoing on
> that
>       same slave.
>       
>       This patch prohibits a slave from starting replication when a restore is in
>       progress.
> modified:
>   mysql-test/suite/rpl/r/rpl_backup.result
>   mysql-test/suite/rpl/t/rpl_backup.test
>   sql/backup/kernel.cc
>   sql/mysql_priv.h
>   sql/mysqld.cc
>   sql/repl_failsafe.cc
>   sql/repl_failsafe.h
>   sql/share/errmsg.txt
>   sql/si_objects.cc
>   sql/si_objects.h
>   sql/sql_repl.cc
>
(...)
> === modified file 'mysql-test/suite/rpl/t/rpl_backup.test'
> --- a/mysql-test/suite/rpl/t/rpl_backup.test	2008-11-17 09:57:51 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_backup.test	2008-12-03 02:53:40 +0000
> @@ -375,6 +375,63 @@ eval SELECT $master_after_pos - $master_
>  --enable_query_log
>  
>  #
> +# Now test 'slave start' while restore is in progress on slave.
> +#
> +
> +RESET MASTER;
> +
> +connection slave;
> +
> +RESET SLAVE;
> +
> +SET DEBUG_SYNC = 'reset';
> +
> +connection slave1;
> +
> +SET DEBUG_SYNC = 'restore_before_end SIGNAL restore_running WAIT_FOR proceed';
> +SEND RESTORE FROM 'rpl_bup_s3.bak' OVERWRITE;
> +
> +connection slave;
> +
> +SET DEBUG_SYNC = 'now WAIT_FOR restore_running';
> +
> +--echo Try to start the slave while restore is running -- gets error.
> +--error ER_RESTORE_CANNOT_START_SLAVE
> +SLAVE START;
> +
> +SET DEBUG_SYNC = 'now SIGNAL proceed';
> +
> +--replace_result $MASTER_MYPORT MASTER_PORT
> +--replace_column 1 # 6 # 7 # 8 # 9 # 22 # 23 # 33 #
> +--query_vertical SHOW SLAVE STATUS
> +
> +connection slave1;
> +--echo Restore is now complete.
> +--replace_column 1 #
> +reap;
> +SET DEBUG_SYNC = 'now SIGNAL done';
> +
> +connection slave;
> +
> +SET DEBUG_SYNC = 'now WAIT_FOR done';

Are you sure this synchronization is needed? Aren't the statements after 
"connection slave" executed only when "reap" completes the RESTORE statement?

> +
> +SHOW DATABASES; 
> +
> +SET DEBUG_SYNC = 'reset';
> +
> +--echo Try to start the slave after restore is done -- should succeed.
> +SLAVE START;
> +--source include/wait_for_slave_to_start.inc
> +
> +--replace_result $MASTER_MYPORT MASTER_PORT
> +--replace_column 1 # 6 # 7 # 8 # 9 # 22 # 23 # 33 #
> +--query_vertical SHOW SLAVE STATUS
> +
> +--echo Now stop the slave.
> +SLAVE STOP;
> +--source include/wait_for_slave_to_stop.inc
> +
> +#
>  # Cleanup
>  #
>  connection master;
> 
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc	2008-11-28 10:10:39 +0000
> +++ b/sql/backup/kernel.cc	2008-12-03 02:53:40 +0000
> @@ -219,10 +219,14 @@ execute_backup_command(THD *thd, LEX *le
>      
>      DEBUG_SYNC(thd, "after_backup_start_restore");
>  
> +    obs::restore_running(TRUE);
> +
>      res= context.do_restore(overwrite);      
>  
>      DEBUG_SYNC(thd, "restore_before_end");
>  
> +    obs::restore_running(FALSE);
> +

[1] These calls, or equivalent, should be done inside context class which is the 
interface to the online backup functionality (see comment at the beginning of 
this file). The user of this interface should not be requested to perform 
actions which are part of its implementation. Instead, the interface should be 
fully implemented bu Backup_restore_ctx class. A user of the interface does:

   Backup_restore_ctx context(...);
   Restore_info *info= context.prepare_for_restore(...);
   context.do_restore(...);
   context.close(...);

and it should work as expected without any further actions from the user.

When moving the calls to Backup_restore_ctx class, please follow the existing 
design pattern, then any changes of the execution context are reversed inside 
Backup_restore_ctx::close() which is also called from destructor. E.g., removing 
the blocking of replication on this server should be done there, so that we are 
sure the block will be always removed if introduced.

>      if (res)
>        DBUG_RETURN(send_error(context, ER_BACKUP_RESTORE));
>      
>
(...)

> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc	2008-11-17 11:17:59 +0000
> +++ b/sql/mysqld.cc	2008-12-03 02:53:40 +0000
(...)
> @@ -3914,6 +3916,10 @@ static int init_server_components()
>    my_uuid_init((ulong) (my_rnd(&sql_rand))*12345,12345);
>  #ifdef HAVE_REPLICATION
>    init_slave_list();
> +  init_slave_start();
> +  pthread_mutex_lock(&LOCK_slave_start);
> +  allow_slave_start= TRUE;
> +  pthread_mutex_unlock(&LOCK_slave_start);

[2] Why not do the initialization of allow_slave_start inside init_slave_start()?

>  #endif
>  
>    /* Setup logs */
> 
> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc	2008-10-30 12:29:54 +0000
> +++ b/sql/si_objects.cc	2008-12-03 02:53:40 +0000
> @@ -4073,6 +4073,22 @@ int disable_slave_connections(bool disab
>  }
>  
>  /**
> +  Set state where restore is running.
> +
> +  This method tells the server that a restore is in progress.
> +  This is used to prohibit slaves from starting once a restore is
> +  in progress.
> +
> +  param[IN] running  TRUE = restore running, FALSE = no restore running
> +*/

[3] I don't see any reason to make this service restore-specific. It could be 
formulated as a general request for blocking starting replication on this 
server. The fact that it is used by RESTORE is not relevant here. Thus a more 
appropriate name could be block_replication or something like this.

> +void restore_running(bool running)
> +{
> +  pthread_mutex_lock(&LOCK_slave_start);
> +  allow_slave_start= !running;
> +  pthread_mutex_unlock(&LOCK_slave_start);
> +}
> +
> +/**
>    Write an incident event in the binary log.
>  
>    This method can be used to issue an incident event to inform the slave
> 
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc	2008-10-28 18:14:14 +0000
> +++ b/sql/sql_repl.cc	2008-12-03 02:53:40 +0000
> @@ -1000,6 +1000,21 @@ int start_slave(THD* thd , Master_info* 
>  
>    if (check_access(thd, SUPER_ACL, any_db,0,0,0,0))
>      DBUG_RETURN(1);
> +
> +  /*
> +    Ensure there are no restores running on the server.
> +  */
> +  pthread_mutex_lock(&LOCK_slave_start);
> +  bool proceed= allow_slave_start;
> +  pthread_mutex_unlock(&LOCK_slave_start);
> +  printf("WHAT??? %d\n", proceed);

This looks like a temporary debug printout -- should be removed in that case.

> +  if (!proceed)
> +  {
> +    slave_errno= ER_RESTORE_CANNOT_START_SLAVE;
> +    my_message(slave_errno, ER(slave_errno), MYF(0));
> +    DBUG_RETURN(1);
> +  }
> +

[4] Here is a (theoretically) possible race condition:

  Thread 1
   |   Thread 2
   |    |
  execute start_slave() which does:
    pthread_mutex_lock(&LOCK_slave_start);
    bool proceed= allow_slave_start; // allow_slave_start is TRUE at this moment
    pthread_mutex_unlock(&LOCK_slave_start);
   |    |
   |   execute restore_running(TRUE) which does:
   |     pthread_mutex_lock(&LOCK_slave_start);
   |     allow_slave_start= !running;
   |     pthread_mutex_unlock(&LOCK_slave_start);
   |    |
  continue start_slave():
    if (!proceed)  <--- Ooops, proceed is TRUE but start_slave should fail!
    {
      slave_errno= ER_RESTORE_CANNOT_START_SLAVE;
      my_message(slave_errno, ER(slave_errno), MYF(0));
      DBUG_RETURN(1);
    }

  This race could happen only if restore_running() is called while the
  server acts as a slave. This is probably not possible within RESTORE code, but
  I think a stronger protection against it should be done because in principle
  si_objects could be used also by other clients.

  Suggested fix: use three values for allow_slave_start: ENABLED, DISABLED,
  RUNNING. Restore_running will only change it to DISABLED/ENABLED if the current
  value is not RUNNING. Otherwise it will report error or, better, crash on
  an assertion (ensuring that it is not called if replication is active). The
  restriction should be clearly documented in specs. See also my suggestion X.

Rafal