Hi Jorgen,
STATUS: Approved.
See some minor comments/questions/suggestions below.
Jorgen Loland wrote:
> #At file:///localhome/jl208045/mysql/backup-brances/mysql-6.0-backup-44568/ based on
> revid:charles.bell@stripped
>
> 2892 Jorgen Loland 2009-11-20
> Bug#44568: Online backup 'waiting' while load executed
>
> To make a transaction consistent backup image across all tables
> and storage engines, backup needs to block commits during the
> synchronization phase. Previously, this was achieved by using
> global read lock (GRL). However, GRL was not intended for this
> usage and has some serious disadvantages:
> - backup did not use it as it was meant to be used, and GRL
> will therefore starve backup even with few concurrent
> transactions
> - backup only needs to block commits/be blocked by commits.
> Backup's usage of GRL blocked DMLs.
>
> This patch contains a temporary solution to the problem. It
> changes the backup metadata locking to a more generic backup
> shared/exclusive locking mechanism that is used for both
> BML and commit blocking. Compared to using GRL for commit
> blocking, much fewer operations are blocked (only commits vs
> whole transactions starting from it's first DML statement).
> Further, the starvation of backup by concurrent transactions
> is removed because the locking mechanism orders lock requests
> FIFO.
>
> The patch is a temporary solution because the locking mechanism
> will most likely be replaced by MetaDataLocking later. There are
> plans for scoped locks in MDL, which will make backup block
> commits only in the tables being backed up.
> === modified file 'sql/bml.cc'
...
> @@ -34,219 +76,248 @@
> TRUNCATE/OPTIMIZE/REPAIR TABLE
>
> The parser (mysql_execute_command() in sql_parse.cc) arranges for calls to
> - bml_enter() and bml_leave() for these statements.
> -*/
> + get_shared_lock() and release_shared_lock() for these statements.
>
> -#include "bml.h"
> -#include "debug_sync.h"
> + BACKUP COMMIT BLOCKER
> + ---------------------
>
> -BML_class *BML_class::m_instance= NULL;
> + To get a transaction consistent image of all tables in all
> + databases being backed up, BACKUP needs to disallow commits for a
> + short period of time to synchronization data from all storage
Typo: "to synchronize"
> + engines. This is implemented by making BACKUP acquire an exclusive lock
> + for the duration of the synchronization phase, and by making all
> + commits of transactions that have modified data acquire Shared locks.
...
> /*
> - Check whether BML is active. If yes, wait for deactivation which is
> - signalled with COND_BML.
> + Check whether an exclusive lock is set. If yes, wait for release
> + which is signalled with COND_no_xlock.
> */
> - pthread_mutex_lock(&THR_LOCK_BML_active);
> - thd->enter_cond(&COND_BML, &THR_LOCK_BML_active,
> - "BML: waiting until released");
> - DEBUG_SYNC(thd, "bml_enter_check");
> - while (BML_active && !thd->killed && !thd->BML_exception
> && (ret == 0))
> + pthread_mutex_lock(&LOCK_mutex);
> + thd->enter_cond(&COND_no_xlock, &LOCK_mutex,
> + "Backup_sx_lock: waiting until exclusive lock is released");
> + while (xlock_set && xlock_set!=thd && !thd->killed &&
> (ret == 0))
> {
> if (thd->backup_wait_timeout == 0)
> + {
> ret = -1;
> + }
> else
> - ret= pthread_cond_timedwait(&COND_BML, &THR_LOCK_BML_active,
> + {
> + /*
> + The below sync point fires if the exclusive lock is set.
> +
> + WARNING: Be careful when using WAIT_FOR with this sync point.
> + We hold LOCK_mutex here.
> + */
> + DEBUG_SYNC(thd, "wait_if_backup_exclusive_locked");
> + ret= pthread_cond_timedwait(&COND_no_xlock, &LOCK_mutex,
> &ddl_timeout);
> + }
> }
> +
I wonder if it would not possible/better to thd->exit_cond() here, after the
while loop. Then look for interruptions, errors etc.
> if (thd->killed)
> {
> - /* Releases THR_LOCK_BML_active */
> - thd->exit_cond("BML: Thread was killed");
> + /* Releases LOCK_mutex */
> + thd->exit_cond("Backup_sx_lock: Thread was killed");
> DBUG_RETURN(FALSE);
> }
>
> - thd->exit_cond("BML: entered");
> if (ret == 0)
> - do_enter();
> + slock_count++;
> else
> my_error(ER_DDL_TIMEOUT, MYF(0), thd->query());
>
> + thd->exit_cond("Backup_sx_lock: entered");
> +
> + DEBUG_SYNC(thd, "backup_got_shared_lock");
> DBUG_RETURN(ret == 0);
> }
>
...
> +my_bool Backup_sx_lock::get_exclusive_lock(THD *thd)
> +{
> + DBUG_ENTER("Backup_sx_lock::get_exclusive_lock()");
> +
> + //part 1 - get the exclusive lock
> + pthread_mutex_lock(&LOCK_mutex);
> + if (unlikely(xlock_set))
> {
> - thd->exit_cond("BML: Thread was killed");
> + /*
> + Should never happen since only one backup/restore operation is
> + allowed at any time
> + */
Hmm, what about the case that xlock_set == thd, that is, the same thread is
getting exclusive lock while already holding it. I'd allow such scenario
(i.e. report success in that case), but it is also OK to leave it as it is.
> + thd->exit_cond("Backup_sx_lock: An exclusive lock has already been set");
> DBUG_RETURN(FALSE);
> }
>
...
> + //part 2 - wait for all shared locks to be released
> + thd->enter_cond(&COND_no_slock, &LOCK_mutex,
> + "Backup_sx_lock: waiting for release of all Shared locks");
> + while (slock_count != 0 && !thd->killed)
> + {
> + /*
> + The below sync point fires if a shared lock is set.
> +
> + WARNING: Be careful when using WAIT_FOR with this sync point.
> + We hold LOCK_mutex here.
> + */
> + DEBUG_SYNC(thd, "wait_backup_exclusive_lock");
> + /* wait in queue that is activated when slock_count drops to 0 */
> + pthread_cond_wait(&COND_no_slock, &LOCK_mutex);
> + }
Suggestion: First exit_cond(), then do result analysis. I do see that you
give different info to exit_cond() depending on the context. However, in
this case I'd prefer safety to verbosity - it is very easy to forget to add
exit_cond() in some branch and then we have potential deadlock which is hard
to investigate.
> if (thd->killed)
> {
> - /* Releases THR_LOCK_BML */
> - thd->exit_cond("BML: Thread was killed");
> - /* This thread still has BML_active set - release it */
> - bml_release();
> + /* Releases LOCK_mutex */
> + thd->exit_cond("Backup_sx_lock: Thread was killed");
> + /* This thread alread got X lock; release it */
> + release_exclusive_lock();
> DBUG_RETURN(FALSE);
> }
>
> - thd->exit_cond("BML: activated");
> + thd->exit_cond("Backup_sx_lock: Exclusive lock set");
>
> - DEBUG_SYNC(thd, "after_bml_activated");
> + DEBUG_SYNC(thd, "after_backup_exclusivelock_set");
> DBUG_RETURN(TRUE);
> }
...
> === modified file 'sql/handler.cc'
...
> @@ -1174,6 +1180,11 @@ int ha_commit_trans(THD *thd, bool all)
> !thd->slave_thread)
> {
> my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "--read-only");
> + if (bcb_registered)
> + {
> + BCB_instance->release_shared_lock();
> + bcb_registered=FALSE;
> + }
I wonder why you need this if below you "goto end" and there the lock will
be released.
> ha_rollback_trans(thd, all);
> error= 1;
> goto end;
> @@ -1210,6 +1221,11 @@ int ha_commit_trans(THD *thd, bool all)
> if (error || (is_real_trans && xid &&
> (error= !(cookie= tc_log->log_xid(thd, xid)))))
> {
> + if (bcb_registered)
> + {
> + BCB_instance->release_shared_lock();
> + bcb_registered=FALSE;
> + }
> ha_rollback_trans(thd, all);
> error= 1;
> goto end;
> @@ -1229,6 +1245,7 @@ end:
> /* Free resources and perform other cleanup even for 'empty' transactions. */
> else if (is_real_trans)
> thd->transaction.cleanup();
> + if (bcb_registered) BCB_instance->release_shared_lock();
> DBUG_RETURN(error);
> }
>
>
Rafal
