List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:November 5 2008 2:59pm
Subject:Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2727)
Bug#32465
View as plain text  
Hi Jorgen,

STATUS
------
Not approved. Requesting rethinking of the test design.

COMMENTARY
----------
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.

===============================================================================
...

   connection con2;

   --echo con2: Get a transaction going and stop in the middle
   --echo Assumption (a): TRX in progress is not included in backup
   BEGIN;
    UPDATE bup_commit_blocker.t1 SET col_a = "con2: CHANGED"
    WHERE col_a LIKE  '01%';

     connection con3;

     --echo con3: Start a transaction and send commit after lock is taken
     --echo Assumption (b): TRX in commit is included in backup
     BEGIN;
      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");

connection con1;
--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";

           connection con6;
           --echo con6: Start INSERT and make it stop after it has locked the
           --echo       tables.
           SET DEBUG_SYNC= 'after_insert_locked_tables SIGNAL insert_started
                            WAIT_FOR finish_insert';
           send INSERT INTO bup_commit_blocker.t4 VALUES (31), (32), (33);

     connection con3;
     --echo con3: Start COMMIT and make it stop in the middle.
     SET DEBUG_SYNC= 'within_ha_commit_trans SIGNAL commit_started
                      WAIT_FOR continue_commit';
     send COMMIT;

connection default;
--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
                  WAIT_FOR bup_finish';
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';

       connection con4;
       --echo Start BEGIN and setup signal notifying that it has started.
       SET DEBUG_SYNC= 'before_begin_trans SIGNAL begin_started';
       send BEGIN;

connection default;
--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?

             connection con7;
             --echo Start DELETE and see that it is blocked by the commit
             --echo blocker.
             SET DEBUG_SYNC= 'wait_for_global_read_lock SIGNAL delete_blocked';
             send DELETE FROM bup_commit_blocker.t5 WHERE col_a = 50;

connection default;
--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.

   connection con2;
   --echo con2: Completing transaction
     DELETE FROM bup_commit_blocker.t2 WHERE col_a LIKE '02%';
   COMMIT;

     connection con3;
     --echo con3: Fetch COMMIT result
     reap;

       connection con4;
       --echo con4: Fetch BEGIN result and completing transaction
       reap;
        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%';
       COMMIT;

           connection con6;
           --echo con6: Fetch INSERT result
           reap;

             connection con7;
             --echo con7: Fetch DELETE result
             reap;

connection con1;
--echo con1: Fetch BACKUP result
--replace_column 1 #
reap;

===============================================================================

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 
deactivation.
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 
blocker itself).

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).

Rafal

Thread
bzr commit into mysql-6.0-backup branch (jorgen.loland:2727) Bug#32465Jorgen Loland5 Nov
  • Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2727)Bug#32465Rafal Somla5 Nov
    • Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2727)Bug#32465Jørgen Løland6 Nov
      • Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2727)Bug#32465Rafal Somla7 Nov