Hi Chuck,
Thanks for your review. I've changed my patch according to your comments. See
the new patch here <http://lists.mysql.com/commits/79324>.
> DETAILS
> -------
>> Bug #43322 - Finish time of BACKUP operation not stored in
>> backup image
>
> [2] Please restate the problem here. I think we need more explanation of
> what the problem was (why mainly) and then how this patch fixes it.
>
I've added this:
"Before this patch, the finish time of BACKUP operation was reported in the
backup logs, but was not stored in the summary section of the backup image.
This was because the time was not yet stored in the Backup_info object at
the time when backup image summary section was written. This patch fixes
this by ensuring that:"
>> Fixed by ensuring that:
>> 1) End time of backup operation is stored in Backup_info
>> *before* backup image
>> summary is written.
>> 2) The times stored in backup image's summary and logged
>> in backup logs are the
>> same.
>> @ mysql-test/suite/backup/t/backup_logs.test
>> Added new test for checking that times stored in backup logs
>> and in backup image's summary section are the same.
>
> [2] I am going to be picky here. It isn't a new test you are adding. As
> Ingo has pointed out, what you are adding is a test case to a test file.
> Please change comment accordingly.
>
No problem. I've changed it to "new test case".
>> @ sql/backup/image_info.h
>> Add get_end_time() method. Move code common to get_*_time() to
>> new
>> helper function read_time() - a counterpart of save_time().
>> @ sql/backup/kernel.cc
>> - When end of backup/restore operation is reported in
>> Backup_info::close() use the end time saved in the
>> Backup_info structure. If the end time was not saved before,
>> save and use the current time.
>> - Save end time after backup of table data is complete and
>> before the summary section is written.
...
>> === modified file 'mysql-test/suite/backup/t/backup_logs.test'
>> --- a/mysql-test/suite/backup/t/backup_logs.test 2009-05-25
>> 07:11:29 +0000
>> +++ b/mysql-test/suite/backup/t/backup_logs.test 2009-07-15
>> 06:42:39 +0000
>> @@ -318,6 +318,35 @@ SELECT timediff(validity_point_time, sta
>> timediff(stop_time, validity_point_time) >=0
>> from mysql.backup_history WHERE backup_id=500;
>>
>> +
>> +--echo Now verify that times from the log are the same as the ones
>> stored in the image.
>> +
>> +# Use mysqlbackup utility to read the summary of the backup image.
>> +exec $MYSQL_BACKUP --summary $MYSQLD_BACKUPDIR/backup_logs_orig.bak
>> >$MYSQLD_DATADIR/backup_logs_orig.summary;
>> +
>> +# Create a table and load output of mysqlbackup into it to access the
>> data.
>> +CREATE TABLE summary (header text, value datetime);
>> +--disable_warnings
>> +eval LOAD DATA INFILE '$MYSQLD_DATADIR/backup_logs_orig.summary'
>> + INTO TABLE summary
>> + FIELDS TERMINATED BY 'time:';
>> +--enable_warnings
>> +--remove_file $MYSQLD_DATADIR/backup_logs_orig.summary
>
> [1] I think you need to either mask out the $MYSQLD_DATADIR or use
> disable/enable to turn the query log off then on again -- see
> loaddata.test for an example.
>
Yes, good catch. I decided to disable query logging for the statements which
load and parse the output of mysqlbackup utility.
Rafal
