Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2727) Bug#32465

[Jørgen Løland]

Thanks for the review Rafal. Comments are inline.

Rafal Somla wrote:
> 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.

(...)

Hmm... an interesting suggestion, but are you sure it works to set sync 
points in a connection that is already processing an operation? Like:

send BACKUP ...
...
SET DEBUG_SYNC ...
...
reap;

Personally, if the tests get complex, I like starting as many clients as 
there are connections, and cut'n paste the test script into the 
different windows. That does not work with the suggested script, but 
maybe it is easier to read.

I suggest to do the following:

1. Add better comments to all operations with sync points, and what they 
are supposed to trigger.
2. Indent 2 spaces for each connection
3. See if any connection can be removed.

If you disagree, and Chuck agrees with you, I'll do as I'm told to :)

> 1. How do we test that things are really blocked by each other. Waiting 
> for reaching given sync point is not a good method.

Why is it not good to wait for sync? I need some suggestion for how to 
do this test if this is a bad idea.

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

I'll look at this again, but I already tried to remove unnecessary 
connections.

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

Regarding commits, I'll see what I can do about scenarios a) and b) above.

However, I think it makes sense to test both non-tx and tx engines in 
the same test file. This test file should test that everything committed 
[1] before VP is restored, but nothing else.

[1] Including non-tx DMLs; I think of these as operations with implicit 
commits. Maybe that's wrong.

-- 
Jørgen Løland