STATUS
------
Patch approved on condition of satisfying the three requests below,
REQUESTS
--------
1. Please clarify comment in patch header or remove it.
2. Please clarify comment in test header.
3. Please add comments for complex debug sync sequence.
COMMENTARY
----------
Good work!
SUGGESTIONS
-----------
None
DETAILS
-------
> Bug#32465 - Backup: Commit blocker does not block
> tables in default driver
>
> This bug has been removed by an unknown fix, but a
> proper test case was never added.
[1] Huh? Please track this fact down or remove the comment.
> The patch contains a commit blocker test written for
> the temporary commit blocker solution of setting Global Read
> Lock before backup prepare phase (fixed in bug#39602).
> modified:
...
> === modified file
> 'mysql-test/suite/backup/t/backup_commit_blocker.test'
> --- a/mysql-test/suite/backup/t/backup_commit_blocker.test
> 2008-10-07 17:15:44 +0000
> +++ b/mysql-test/suite/backup/t/backup_commit_blocker.test
> 2008-11-04 07:42:47 +0000
> @@ -61,14 +61,15 @@ connect (con5,localhost,root,,);
> connect (con6,localhost,root,,);
> connect (con7,localhost,root,,);
>
> -#
> -# Note: Originally we used SELECT ... FROM
> INFORMATION_SCHEMA.PROCESSLIST; to show status of
> -# the various connections participating in the test. But
> INFORMATION_SCHEMA.PROCESSLIST implementation
> -# proved to be not relaiable. From time to time
> (nondeterminisrtically) test hanged on these SELECTs.
> -# As showing the status is not essential for the test (we
> test correctness by checking the contents of
> -# the created backup image), the selects are commented out
> now. They can be re-enabled when
> -# I_S.PROCESSLIST is in a better shape.
> -#
> +# Note: Originally we used SELECT ... FROM
> +# INFORMATION_SCHEMA.PROCESSLIST; to show status of the various
> +# connections participating in the test. But
> +# INFORMATION_SCHEMA.PROCESSLIST implementation proved to be not
> +# relaiable. From time to time (nondeterminisrtically) test hanged on
[2a] Choice of 'hanged' is the wrong word. You should use 'hung'.
> +# these SELECTs. As showing the status is not essential for the test
> +# (we test correctness by checking the contents of the created backup
> +# image), the selects are commented out now. They can be re-enabled
> +# when I_S.PROCESSLIST is in a better shape.
[2b] Comment says the selects were commented out but patch removes them.
Which is correct?
>
> connection con1;
>
> @@ -187,16 +188,10 @@ SET DEBUG_SYNC= 'before_backup_unblock_c
> --echo con1: Backing up database -- will block with lock
> send BACKUP DATABASE bup_commit_blocker TO "bup_commit_blocker.bak";
>
> -connection con5;
> -
> ---echo con5: Wait for BACKUP to reach its synchronization point.
> -SET DEBUG_SYNC= 'now WAIT_FOR bup_commit_block';
> -#SELECT state, info FROM
> -#INFORMATION_SCHEMA.PROCESSLIST
> -#WHERE info LIKE "BACKUP DATABASE%";
> -
[2c] Comment says these are commented out but here they are removed.
...
> connection con2;
> @@ -585,128 +534,59 @@ connection con1;
> # the commit blocker.
> #
> --echo con1: Activate synchronization points for BACKUP.
> -# Before blocking commits, pause to show processlist state.
> -# Before going to wait for the protection against global read lock to
> -# disappear, signal about the wait, so that another thread
> can continue.
> -# When COMMIT finishes, it releases the protection against
> global read lock,
> -# which would allow BACKUP to continue while COMMIT also
> continues and
> -# signals "commit_done". It is important that BACKUP does not run in
> -# parallel and send another signal without waiting for the
> commit_done
> -# signal to be processed. So BACKUP itself has to wait for
> that signal
> -# after awaking and taking the global read lock. Only then BACKUP
> -# can proceed and send a signal telling that it took the global read
> -# lock. Unfortunately DEBUG_SYNC does not allow a sync point
> to emit a
> -# signal after a wait_for action. So we need to send the
> signal in a later
> -# sync point. For this test it is not important where this
> signal is sent,
> -# as long as it is sent while BACKUP holds the global read lock.
> -# Here we do it "before_backup_unblock_commit", where we have to
> -# synchronize anyway.
> -SET DEBUG_SYNC= 'before_commit_block SIGNAL bup_commit_block
> - WAIT_FOR bup_go_read_lock';
> -SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL bup_read_lock';
> -SET DEBUG_SYNC= 'before_backup_data_lock WAIT_FOR commit_done';
> -SET DEBUG_SYNC= 'before_backup_unblock_commit SIGNAL bup_read_locked
> - WAIT_FOR finish';
> +SET DEBUG_SYNC= 'before_commit_block SIGNAL bup_started
> WAIT_FOR bup_take_lock';
> +SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL
> bup_wait_read_lock';
> +SET DEBUG_SYNC= 'before_backup_data_lock SIGNAL bup_commit_blocked
> + WAIT_FOR bup_unblock_commit';
[3] Seems like the original comment was warranted. Please write a similar
description here so that we all know what is going on and why we have a
complex sequence of debug synch points.
...
Chuck
