Re: bzr commit into mysql-6.0-backup branch (rsomla:2688) Bug#38261

[Øystein Grøvlen]

Rafal Somla wrote:
> Thanks for looking at the patch!
> 
> Øystein Grøvlen wrote:
>>
>> 1. You have added select statements after each backup, but what does
>>    these statements test?  For restore, rollback is used to test that
>>    restore transaction was automatically committed.  Maybe that could
>>    be done for backup, too?
>>
> 
> These selects also test if transaction has been commited. Since we 
> insert data inside the transaction, select should show it only if 
> transaction is commited.

No. As long as you are using the same connection also uncomitted data 
will be visible.

> 
> The advantage over testing commit with ROLLBACK is that we can continue 
> with transaction commited (although not sure if that is really needed 
> here).
> 
>> 2. You have only added tests for one storage engine/backup driver,
>>    while there a restore tests for a combination of drivers and
>>    autocommit on/off.  Is that sufficient?  Should the behavior of
>>    other engines also be tested?  For example, parts of the test would
>>    fail on Falcon since it does not allow restore of objects locked by
>>    other transactions.
>>
> 
> Jorgen has a similar concern. I'll ponder and see if I can rewrite this 
> test to use "combinations", as Hema did in her tests. Then the same test 
> will be run for the two transactional engines we have in the server. Do 
> you agree that it is a good idea?

I think that is a good idea, and I guess the original test cases that I 
wrote for this test should have been written in a similar fashion.

> 
> 
>> SUGGESTIONS
>> -----------
>>
>> 1. Maybe a comment should be added to close() method to explain why
>>    commit is not needed
>>
> 
> Hmm, I'm reluctant to comment code which is *not* there. I'll see if I 
> can find a good place explaining how commits are handled by 
> BACKUP/RESTORE commands.

Sounds good.

-- 
Øystein