Not approved. Requesting rethinking of the test design.
After initial look at the patch, I have some concerns. I know you are not the
original author of this test, but I believe it is a high time to clean the test
before it becomes a maintenance nightmare...
My main concern is that it is nearly impossible to get an idea of what is going
on in the test. Maybe I'm too stupid to read it, but I think the structure and
documentation can be improved a bit.
Instead of elaborating on what could be done differently, I insert here Test 3
(without preamble) rephrased in a way which I consider more easy to read. I
think that my version is completely equivalent to what is done in the original
test - let me know if you think otherwise.
--echo con2: Get a transaction going and stop in the middle
--echo Assumption (a): TRX in progress is not included in backup
UPDATE bup_commit_blocker.t1 SET col_a = "con2: CHANGED"
WHERE col_a LIKE '01%';
--echo con3: Start a transaction and send commit after lock is taken
--echo Assumption (b): TRX in commit is included in backup
INSERT INTO bup_commit_blocker.t2 VALUES ("con3: 04 Some data to test");
INSERT INTO bup_commit_blocker.t2 VALUES ("con3: 05 Some data to test");
--echo con1: Start BACKUP and make it stop after reading metadata but before
--echo commit blocker is activated
SET DEBUG_SYNC= 'before_commit_block SIGNAL bup_started WAIT_FOR bup_take_lock';
send BACKUP DATABASE bup_commit_blocker TO "bup_commit_blocker.bak";
--echo con6: Start INSERT and make it stop after it has locked the
SET DEBUG_SYNC= 'after_insert_locked_tables SIGNAL insert_started
send INSERT INTO bup_commit_blocker.t4 VALUES (31), (32), (33);
--echo con3: Start COMMIT and make it stop in the middle.
SET DEBUG_SYNC= 'within_ha_commit_trans SIGNAL commit_started
--echo Wait for statements to reach their synchronization points.
SET DEBUG_SYNC= 'now WAIT_FOR bup_started';
SET DEBUG_SYNC= 'now WAIT_FOR insert_started';
SET DEBUG_SYNC= 'now WAIT_FOR commit_started';
--echo Let BACKUP continue but make it stop after commit blocker has been
--echo activated (before_backup_data_lock). Note that it should stop on
--echo wait_for_global_lock, being blocked by ongoing INSERT.
SET DEBUG_SYNC= 'before_backup_data_lock SIGNAL bup_commit_blocked
SET DEBUG_SYNC= 'wait_for_global_read_lock SIGNAL bup_wait_read_lock';
SET DEBUG_SYNC= 'now SIGNAL bup_take_lock';
--echo Check that BACKUP is blocked by INSERT.
SET DEBUG_SYNC= 'now WAIT_FOR bup_wait_read_lock';
--echo Let COMMIT continue until it has committed, but stop it at the very end.
SET DEBUG_SYNC= 'after_commit SIGNAL commit_done WAIT_FOR finish_commit';
SET DEBUG_SYNC= 'now SIGNAL continue_commit';
SET DEBUG_SYNC= 'now WAIT_FOR commit_done';
--echo Now BACKUP should have activated commit blocker.
SET DEBUG_SYNC= 'now WAIT_FOR bup_commit_blocked';
--echo Start BEGIN and setup signal notifying that it has started.
SET DEBUG_SYNC= 'before_begin_trans SIGNAL begin_started';
--echo Check that BEGIN has started and let INSERT finish its job.
SET DEBUG_SYNC= 'now WAIT_FOR begin_started';
SET DEBUG_SYNC= 'now SIGNAL finish_insert';
--echo Show that INSERT statement has executed before backup.
SELECT * FROM bup_commit_blocker.t4;
SELECT * FROM bup_commit_blocker.t5; # why this?
--echo Start DELETE and see that it is blocked by the commit
SET DEBUG_SYNC= 'wait_for_global_read_lock SIGNAL delete_blocked';
send DELETE FROM bup_commit_blocker.t5 WHERE col_a = 50;
--echo See that DELETE is blocked by commit blocker.
SET DEBUG_SYNC= 'now WAIT_FOR delete_blocked';
# Hmm, I'd select from t5 to see that DELETE haven't done anything
--echo Let BACKUP finish its job, releasing the commit blocker.
SET DEBUG_SYNC= 'now SIGNAL bup_unblock_commit';
# Reconnect to con2, con3, con4, con6, and con7 and fetch their results.
--echo con2: Completing transaction
DELETE FROM bup_commit_blocker.t2 WHERE col_a LIKE '02%';
--echo con3: Fetch COMMIT result
--echo con4: Fetch BEGIN result and completing transaction
UPDATE bup_commit_blocker.t3 SET col_a = "con4: 05 CHANGED"
WHERE col_a LIKE '05%';
UPDATE bup_commit_blocker.t3 SET col_a = "con4: 06 CHANGED"
WHERE col_a LIKE '06%';
--echo con6: Fetch INSERT result
--echo con7: Fetch DELETE result
--echo con1: Fetch BACKUP result
--replace_column 1 #
Looking at this I can see few unclear things:
1. How do we test that things are really blocked by each other. Waiting for
reaching given sync point is not a good method.
2. Why do we have BEGIN command - what does that have to do with testing commit
blocker (COMMIT != BEGIN)?
3. We could use fewer connections making test easier to maintain, perhaps.
However, my main concern is that I think this test does not test what it should.
I raised this issue few times before but so far no one seems to understand me.
I have elaborated on this in an email thread with subject "Commit blocker
testing" cc-ed to dev-backup and taking place months 07-08/2008. In short:
The task of commit blocker is to block ongoing COMMITS. It also ensures that VP
is not created while COMMITs are running. The basic properties:
a) when activated, COMMITs can do not commit transactions but wait for its
b) if there are ongoing COMMITs, activating commit blocker will wait for them to
finish and at the same time prevent new COMMITs from happening.
I don't see these things being tested in this test (and I see a lot of other
things tested, which in my opinion are not that much related to the commit
Thus my request would be to:
1. Either change backup_commit_blocker test so that it test the above basic
properties of the commit blocker. If doing this, I suggest to simplify it by not
testing anything else (leaving it for other tests).
2. Or, rename and redefine the purpose of this test. As it is now, it does not
test the commit blocker but rather tests the property that backup image should
contain only committed data and only finished non-trx statements. In that case
make sure that testing the commit blocker is on our roadmap (WL#4135 most
probably - but not sure if the above properties are tested in this WL either).