Code changes look good, but I miss a test for this. Any particular
reason for why a test case similar to what is described in the bug
report can not be added?
--
Øystein
Jorgen Loland wrote:
> #At file:///localhome/jl208045/mysql/mysql-6.0-backup-39602/
>
> 2717 Jorgen Loland 2008-10-24
> Bug#39602 - Backup: deadlock between backup and myisam
>
> Backup code sets table locks in MyISAM and default drivers before blocking DMLs
> in the commit blocker. DML operations do this in reverse order. A deadlock could therefore
> occur.
>
> The patch moves the commit blocker before the table locks are set, thereby
> removing the potential for a deadlock. This is a temporary solution. WL#4610 is about
> implementing a refined commit blocker without this problem.
> modified:
> sql/backup/data_backup.cc
>
> per-file messages:
> sql/backup/data_backup.cc
> Acquire commit blocker before prepare phase to avoid possibility of deadlock with
> DMLs when using MyISAM or default backup driver.
> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc 2008-10-14 12:08:56 +0000
> +++ b/sql/backup/data_backup.cc 2008-10-24 13:16:38 +0000
> @@ -552,6 +552,25 @@ int write_table_data(THD* thd, Backup_in
> DBUG_PRINT("backup_data",("-- PREPARE PHASE --"));
> DEBUG_SYNC(thd, "before_backup_data_prepare");
>
> + /*
> + Note: block_commits is performed here because of the global read
> + lock/table lock deadlock reported in bug#39602. It should be
> + moved back to right before sch.lock() once a refined commit
> + blocker has been implemented. WL#4610 tracks the work on a
> + refined commit blocker
> + */
> + /*
> + Block commits.
> +
> + TODO: Step 2 of the commit blocker has been skipped for this release.
> + When it is included, developer needs to build a list of all of the
> + non-transactional tables and pass that to block_commits().
> + */
> + int error= 0;
> + error= block_commits(thd, NULL);
> + if (error)
> + goto error;
> +
> if (sch.prepare())
> goto error;
>
> @@ -574,16 +593,8 @@ int write_table_data(THD* thd, Backup_in
> DEBUG_SYNC(thd, "after_backup_validated");
>
> /*
> - Block commits.
> -
> - TODO: Step 2 of the commit blocker has been skipped for this release.
> - When it is included, developer needs to build a list of all of the
> - non-transactional tables and pass that to block_commits().
> + Refined commit blocker should be set here; see WL#4610
> */
> - int error= 0;
> - error= block_commits(thd, NULL);
> - if (error)
> - goto error;
>
> DEBUG_SYNC(thd, "before_backup_data_lock");
> if (sch.lock())
>
>
