Hi Chuck,
I have found few minor problems,
please see below. Also, it will be better if
the commit message includes the diff of
all changes for the bug fix instead of incremental
changes from the last patch.
Status
-----------
Not approved (yet)
Required
----------------
[1] Error message needs change. [ cut & paste problem?]
[2] The backup id argument missing in help message.
[3] The backup date argument missing in help message.
[4] Need testing of purge backup logs
[5] Need testing of purge backup logs id/date.
[6] From the earlier patch, following comment needs to be removed :
# 'purge backup logs' reopens log files.
Suggestions
---------------------
It is not clear from the help message that backup date argument
could be date/timestamp as permitted by the current locale of server.
I will make this clear in the help message even if it means the help
message spans more than single line.
-thava
Chuck Bell wrote:
> #At file:///Users/cbell/source/bzr/mysql-6.0-bug-48353/ based on
> revid:charles.bell@stripped
>
> 2891 Chuck Bell 2009-11-13
> BUG#48353 : Add PURGE BACKUPLOGS to mysqladmin
>
> It has been decided the PURGE BACKUP LOGS is a companion
> to the FLUSH BACKUP LOGS command and should be added to
> the options for mysqladmin.
>
> This patch adds the PURGE BACKUP LOGS command. There are
> three forms of this command:
>
> PURGE BACKUP LOGS
> PURGE BACKUP LOGS TO <id>
> PURGE BACKUP LOGS BEFORE <date>
>
> This patch adds the following options to mysqladmin:
>
> purge-backup-logs Purge backup logs
> purge-backup-logs-id Purge backup logs up to a given backup_id
> purge-backup-logs-date Purge backup logs before a given date
>
> Note: WL#5165 was created to provide more advanced testing of
> FLUSH/PURGE BACKUP LOGS.
> @ client/mysqladmin.cc
> Added new commands to mysqladmin.
> @ mysql-test/suite/backup/r/backup_logs_output.result
> Corrected result file.
> @ mysql-test/suite/backup/t/backup_logs_output.test
> Added tests for new commands.
>
> modified:
> client/mysqladmin.cc
> mysql-test/suite/backup/r/backup_logs_output.result
> mysql-test/suite/backup/t/backup_logs_output.test
> === modified file 'client/mysqladmin.cc'
> --- a/client/mysqladmin.cc 2009-11-13 01:54:16 +0000
> +++ b/client/mysqladmin.cc 2009-11-13 15:01:28 +0000
> @@ -98,7 +98,7 @@ enum commands {
> ADMIN_PING, ADMIN_EXTENDED_STATUS, ADMIN_FLUSH_STATUS,
> ADMIN_FLUSH_PRIVILEGES, ADMIN_START_SLAVE, ADMIN_STOP_SLAVE,
> ADMIN_FLUSH_THREADS, ADMIN_OLD_PASSWORD, ADMIN_FLUSH_BACKUP_LOGS,
> - ADMIN_PURGE_BACKUP_LOGS
> + ADMIN_PURGE_BUP_LOGS, ADMIN_PURGE_BUP_LOGS_ID, ADMIN_PURGE_BUP_LOGS_DATE
> };
> static const char *command_names[]= {
> "create", "drop", "shutdown",
> @@ -109,7 +109,7 @@ static const char *command_names[]= {
> "ping", "extended-status", "flush-status",
> "flush-privileges", "start-slave", "stop-slave",
> "flush-threads", "old-password", "flush-backup-logs",
> - "purge-backup-logs",
> + "purge-backup-logs", "purge-backup-logs-id", "purge-backup-logs-date",
> NullS
> };
>
> @@ -897,7 +897,14 @@ static int execute_commands(MYSQL *mysql
> }
> break;
> }
> - case ADMIN_PURGE_BACKUP_LOGS:
> + /*
> + We have three forms of this command:
> +
> + PURGE BACKUP LOGS
> + PURGE BACKUP LOGS TO <id>
> + PURGE BACKUP LOGS BEFORE <date>
> + */
> + case ADMIN_PURGE_BUP_LOGS:
> {
> if (mysql_query(mysql,"purge backup logs"))
> {
> @@ -907,6 +914,43 @@ static int execute_commands(MYSQL *mysql
> }
> break;
> }
> + case ADMIN_PURGE_BUP_LOGS_ID:
> + {
> + char buff[FN_REFLEN+20];
> + if (argc < 2)
> + {
> + my_printf_error(0, "Too few arguments to purge backup logs to",
> + error_flags);
>
> + return 1;
> + }
> + sprintf(buff,"PURGE BACKUP LOGS TO %s", argv[1]);
> + if (mysql_query(mysql, buff))
> + {
> + my_printf_error(0,"refresh failed; error: '%s'", error_flags,
> + mysql_error(mysql));
>
[1] The message needs to be changed. Cut & paste error ?
> + return -1;
> + }
> + argc--; argv++;
> + break;
> + }
> + case ADMIN_PURGE_BUP_LOGS_DATE:
> + {
> + char buff[FN_REFLEN+20];
> + if (argc < 2)
> + {
> + my_printf_error(0, "Too few arguments to purge backup logs before",
> + error_flags);
> + }
> + sprintf(buff,"PURGE BACKUP LOGS BEFORE '%s'", argv[1]);
> + if (mysql_query(mysql, buff))
> + {
> + my_printf_error(0,"refresh failed; error: '%s'", error_flags,
> + mysql_error(mysql));
>
Same as [1]
> + return -1;
> + }
> + argc--; argv++;
> + break;
> + }
> case ADMIN_FLUSH_STATUS:
> {
> if (mysql_query(mysql,"flush status"))
> @@ -1118,8 +1162,10 @@ static void usage(void)
> extended-status Gives an extended status message from the server\n\
> flush-hosts Flush all cached hosts\n\
> flush-logs Flush all logs\n\
> - flush-backup-logs Flush backup logs\n\
> - purge-backup-logs Purge backup logs\n\
> + flush-backup-logs Flush backup logs\n\
> + purge-backup-logs Purge backup logs\n\
> + purge-backup-logs-id Purge backup logs up to a given backup_id\n\
>
[2] The syntax should highlight the expected argument format, for
example:
purge-backup-logs-id <numeric-id> Purge backup logs up to
given backup id.
This makes it clear that there should be no "=" after the option.
> + purge-backup-logs-date Purge backup logs before a given date\n\
>
[3] The argument format specified for date as well. For example:
purge-backup-logs-date <date> ....
It is not clear from above that either date or timestamp (as per
current locale setting) is accepted. I would make this clear in
the explanation.
> flush-status Clear status variables\n\
> flush-tables Flush all tables\n\
> flush-threads Flush the thread cache\n\
>
> === modified file 'mysql-test/suite/backup/r/backup_logs_output.result'
> --- a/mysql-test/suite/backup/r/backup_logs_output.result 2009-11-13 01:54:16 +0000
> +++ b/mysql-test/suite/backup/r/backup_logs_output.result 2009-11-13 15:01:28 +0000
> @@ -241,6 +241,24 @@ Test flush-backup-logs from mysqladmin
> #
> # Test purge-backup-logs from mysqladmin
> #
> +#
> +# Do another backup here then try next version of purge backup logs
> +#
> +BACKUP DATABASE bup_log TO 'bup_log.bak';
> +backup_id
> +#
> +#
> +# Test purge-backup-logs-to from mysqladmin
> +#
> +#
> +# Do another backup here then try next version of purge backup logs
> +#
> +BACKUP DATABASE bup_log TO 'bup_log.bak';
> +backup_id
> +#
> +#
> +# Test purge-backup-logs-date from mysqladmin
> +#
> SET GLOBAL backup_history_log=0;
> SET GLOBAL backup_progress_log=0;
> Should show 'OFF'
>
> === modified file 'mysql-test/suite/backup/t/backup_logs_output.test'
> --- a/mysql-test/suite/backup/t/backup_logs_output.test 2009-11-13 01:54:16 +0000
> +++ b/mysql-test/suite/backup/t/backup_logs_output.test 2009-11-13 15:01:28 +0000
> @@ -276,6 +276,30 @@ FLUSH BACKUP LOGS;
> --echo #
> --exec $MYSQLADMIN --no-defaults --default-character-set=latin1 -S $MASTER_MYSOCK -P
> $MASTER_MYPORT -u root --password= purge-backup-logs 2>&1
>
[4] Need testing of purge backup logs.
Since the files are created as zero sized files, you can just print
the log files here.
>
> +--echo #
> +--echo # Do another backup here then try next version of purge backup logs
> +--echo #
> +--remove_file $MYSQLD_BACKUPDIR/bup_log.bak
> +--replace_column 1 #
> +BACKUP DATABASE bup_log TO 'bup_log.bak';
> +
> +--echo #
> +--echo # Test purge-backup-logs-to from mysqladmin
> +--echo #
> +--exec $MYSQLADMIN --no-defaults --default-character-set=latin1 -S $MASTER_MYSOCK -P
> $MASTER_MYPORT -u root --password= purge-backup-logs-id 99999 2>&1
> +
>
[5] Need testing of backup-logs-id. I observed that this command does not
really do anything to the logs but just cleans up entries in the
tables.
Not sure if that was intended design for the SQL command.
In any case, I would include something like:
select * from mysql.backup_progress;
select * from mysql.backup_history;
> +--echo #
> +--echo # Do another backup here then try next version of purge backup logs
> +--echo #
> +--remove_file $MYSQLD_BACKUPDIR/bup_log.bak
> +--replace_column 1 #
> +BACKUP DATABASE bup_log TO 'bup_log.bak';
> +
> +--echo #
> +--echo # Test purge-backup-logs-date from mysqladmin
> +--echo #
> +--exec $MYSQLADMIN --no-defaults --default-character-set=latin1 -S $MASTER_MYSOCK -P
> $MASTER_MYPORT -u root --password= purge-backup-logs-date '2024-12-31 23:15:10'
> 2>&1
> +
>
Needs testing of purge-backup-logs-date. Same comments as [5].
> --file_exists $MYSQLD_DATADIR/backup_history.log
> --file_exists $MYSQLD_DATADIR/backup_progress.log
>
>
>
> ------------------------------------------------------------------------
>
>
