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
