Rafal,
Comments on the remaining items of note from your review. Unless
otherwise mentioned, all other requests not previously discussed in
separate emails have been fulfilled.
New patch will be ready later today.
> 3. When executing SQLCOM_PURGE_BUP_* commands, do privilege checks
> before anything else, so that a user without enough privileges will not
> see any other error messages.
Ok, one SQLCOM_PURGE_BACKUP_LOGS + lex->type.
> 4. Fix error handling inside scan_backup_log() - with current logic
> errors will be ignored (see inlined comments).
Corrected.
> 5. Remove fixed computations from inside the loop in scan_backup_log()
> (see inlined comments).
Done. My bonehead maneuver. :|
Note: 6. & 7. not applicable to this bug any longer. Noted in WL#4608.
> 8. Fix the documentation of scan_backup_log() function as explained
> below. In particular make it clear which rows exactly are being removed
> by the function.
Done. Code was refactored as well to make it a bit easier to
read/understand/use.
> 9. Explain which row is deleted by delete_row() method.
Done in documentation.
> 11. Please test the error replies from PURGE BACKUP ... statements.
Code refactored and included in the test.
> 12. I'd like to see a test checking what happens if PURGE BACKUP ... and
> BACKUP/RESTORE commands run in parallel.
Done. See #11.
...
>> + if (my_time_compare(&time, &tmp) < 0)
>> + {
>> + if (delete_log_entries) // delete log row
>> + {
>> + /*
>> + When deleting by date for the backup_history table,
>> + we must also delete rows from the backup_progress table
>> + for this backup id.
>> + */
>
> Why we don't do this when delteting by backup_id?
>
> Please mention this behaviour in the documentation of the function.
Explained. See (in new patch) LOGGER::purge_backup_logs in log.cc.
> An idea for alternative implementation: Define a trigger on
> backup_history table which deletes corresponding entries in
> backup_progress whenever an entry is deleted from backup_history.
Can't due to is_log_table restriction.
...
>> === modified file 'sql/sql_parse.cc'
>> --- a/sql/sql_parse.cc 2008-09-26 16:30:56 +0000
>> +++ b/sql/sql_parse.cc 2008-10-18 22:10:41 +0000
>> @@ -2098,6 +2098,147 @@ mysql_execute_command(THD *thd)
>> res = purge_master_logs_before_date(thd, (ulong)it->val_int());
>> break;
>> }
>> + /*
>> + Check log status to see if the command applies.
>> + */
>> + case SQLCOM_PURGE_BUP_LOG_DATE:
>> + case SQLCOM_PURGE_BUP_LOG_ID:
>> + case SQLCOM_PURGE_BUP_IMG_DATE:
>> + case SQLCOM_PURGE_BUP_IMG_ID:
>> + {
>> + char buff[256];
>> + int num= 0;
>> + res= 0;
This portion of the code has been extensively refactored. See new patch.
Chuck
