Re: bzr commit into mysql-6.0-backup branch (cbell:2682) WL#4296

[Jørgen Løland]

Chuck Bell wrote:
> #At file:///C:/source/bzr/mysql-6.0-wl-4296/
> 
>  2682 Chuck Bell	2008-08-14
>       WL#4296 : Online Backup : Refine backup progress mechanism
>       
>       This worklog changes the existing backup progress mechanism from MyISAM
>       tables to logging capability similar to the query log. The new mechanism
>       allows users to specify the destination of the log as a file, table, both,
>       and allows users to turn the logs on or off.

Chuck,

Considering the size of this patch (about one rain forest of printing 
paper) I have very few comments. I think your code is easy to read 
(although this patch was a tad too big). Good work!

Requirements
------------
1. backup_errors writes warnings to var/log/warnings [1]. This warning
    should be ignored
2. Add a test for the backup id codepath that reads the if from file
3. Fix backup<->restore operation bug [2]
4. Test funcs_1/is_columns_mysql fails. You seem to have missed
    the latin1 -> utf8 change in the table definition that was
    pushed last week. The mysql_system_tables.sql file indicates
    that utf8 is in, so maybe you only need to do a make clean
5. backup_logs_output.test: history log is on when echo says it's
    off, progress log is on when echo says it's off.
6. backup_logs_output.test: the --file_exists tests for *.log
    does not test that any information goes into the log files. If
    possible, add a test for this. Disregard otherwise


Suggestions
-----------
1. backup_logs.test does a "DROP DATABASE IF EXISTS backup_logs;
DROP TABLE backup_logs.t1_res;". Wouldn't it be better to drop
table before database?
2. logger.h@236 String str is not used and can be removed.
3. Since this patch touches a lot of server code, I suggest you
do a complete test run of all suites if you haven't already.

[1]
$ cat var/log/warnings
master.err: main.backup_errors: 080819 14:17:43 [ERROR] Can't open the 
backup log tables. Check 'mysql.backup_history' and 'mysql.backup_progress'.
master.err: main.backup_errors: 080819 14:17:43 [ERROR] Can't open the 
backup log tables. Check 'mysql.backup_history' and 'mysql.backup_progress'.

[2] In backup_progress.result, the queries show "operation
restore" after backup and "operation backup" after restore. I
think this is due to two different enumeration sets for
BACKUP/RESTORE.

In logger.h:
enum enum_type { BACKUP, RESTORE } m_type;

In si_logs.h:
enum enum_backup_operation {
   OP_BACKUP = 1,
   OP_RESTORE,
   OP_SHOW,
   OP_OTHER
};

In Logger::init, the Backup_log object is created by casting the
first enum type to the other, but that is incorrect since BACKUP
is 0 in the first one and 1 in the second. Therefore, the
operations are switched in the constructor of Backup_log:

si_logs.cc@41:
   m_op_hist.operation= type ? OP_BACKUP : OP_RESTORE;
   // bug because type == BACKUP == 0 != OP_BACKUP

-- 
Jørgen Løland