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

[Øystein Grøvlen]

STATUS
------

Patch looks OK.  No requests so far, but would like answers to some
questions in commentary before approving patch.

REQUESTS
--------
None, so far.

COMMENTARY
----------

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?

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.

SUGGESTIONS
-----------

1. Maybe a comment should be added to close() method to explain why
    commit is not needed

DETAILS
-------

No further comments.

--
Øystein


Rafal Somla wrote:
> #At file:///ext/mysql/bzr/backup/bug38261/
> 
>  2688 Rafal Somla	2008-09-03
>       BUG#38261 (Online backup: BACKUP command does implicit commit)
>       
>       It was clarified that it is OK for BACKUP to perform implicit commit, so it 
>       is not a problem. Also, the server was modified so that SQL commands marked
> with 
>       CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
>       beginning and at the end of execution. The BACKUP and RESTORE commands are 
>       already marked with the flag.
>       
>       Hence, there is no need for doing any commits inside backup kernel code - calls
> 
>       to ha_autocommit_or_rollback() and end_active_trans() are removed from
> kernel.cc 
>       by this patch.
>       
>       The backup_commit_restore test is extended to check that also BACKUP does
>       an implicit commit. A new test scenario is added to check how BACKUP and
>       RESTORE commands interact with transactions running in other connections. 
> modified:
>   mysql-test/r/backup_commit_restore.result
>   mysql-test/t/backup_commit_restore.test
>   sql/backup/kernel.cc
> 
> per-file messages:
>   mysql-test/t/backup_commit_restore.test
>     - add selects to check that BACKUP commits active transaction
>     - add new test scenario with several connections
>     - add more clean-up (deleting backup image files)
>   sql/backup/kernel.cc
>     Remove code performing commit.
...