Hi Chuck,
General comment: There seems to be a majority for a different approach
regarding the RESTORE statement. I can agree with it, since it is
possible to append /*comment*/ to the RESTORE statement to express a
restore-specific comment, which becomes part of the logged command. So
there is no urgent need for a COMMENT option in RESTORE. However, this
could later be used to be compared with the backup comment from the
image file. But since a new image format is not due now, I don't know,
if the option should be introduced now. I am neutral on this.
Status: Not approved. Fixes required.
Requests:
[1] You always select "command" in front of "user_comment". That way one
does not clearly see if "COMMENT "Test 2"" belongs to "command" or
"user_comment". Please do either have a SELECT for "user_comment" only,
or, better yet, prepend "query_vertical" in front of the SQL statement.
[2] Please keep the lines in the test files <80 characters, wherever
possible.
[5] Do not put space after '*' when '*' introduces a pointer.
[6] Reversed condition.
[8] We do not have parameter types in function comments.
[9] Insufficient function comment.
Suggestions:
[3] Please add parameter names.
[4] Please do not add trailing space to new lines.
[10] You don't need the differentiation anyway. You can just
backup_log->comment(str) unconditionally.
Hints:
[7] So you still want all options in a fixed order.
Details:
Chuck Bell, 15.03.2009 20:57:
...
> 2801 Chuck Bell 2009-03-15
> BUG#43515 : There is no way to use user_comment option from backup_history log
>
> This patch adds the user_comment to the RESTORE and BACKUP
> commands.
...
> --- a/mysql-test/suite/backup/r/backup_logs.result 2009-02-26 22:19:09 +0000
> +++ b/mysql-test/suite/backup/r/backup_logs.result 2009-03-15 19:57:33 +0000
...
> -SELECT operation,num_objects, username, command FROM mysql.backup_history
> +SELECT operation, num_objects, username, command, user_comment FROM
> mysql.backup_history
> WHERE backup_id=@bup_id;
> -operation num_objects username command
> -backup 1 tom BACKUP DATABASE backup_logs TO 'backup_logs1.bak'
> +operation num_objects username command user_comment
> +backup 1 tom BACKUP DATABASE backup_logs TO 'backup_logs1.bak' COMMENT "Test 2"
[1] You always select "command" in front of "user_comment". That way one
does not clearly see if "COMMENT "Test 2"" belongs to "command" or
"user_comment". Please do either have a SELECT for "user_comment" only,
or, better yet, prepend "query_vertical" in front of the SQL statement.
...
> --- a/mysql-test/suite/backup/t/backup_logs.test 2009-03-09 14:00:03 +0000
> +++ b/mysql-test/suite/backup/t/backup_logs.test 2009-03-15 19:57:33 +0000
...
> @@ -132,7 +132,7 @@ reap;
> --echo Get last backup_id
> SELECT MAX(backup_id) INTO @bup_id FROM mysql.backup_history
> WHERE command LIKE "BACKUP DATABASE backup_logs TO%";
> -SELECT operation,num_objects, username, command FROM mysql.backup_history
> +SELECT operation, num_objects, username, command, user_comment FROM
> mysql.backup_history
[2] Please keep the lines in the test files <80 characters, wherever
possible.
...
> --- a/sql/backup/backup_kernel.h 2009-02-20 16:40:19 +0000
> +++ b/sql/backup/backup_kernel.h 2009-03-15 19:57:33 +0000
> @@ -74,10 +74,12 @@ public:
>
> Backup_info* prepare_for_backup(String *location,
> LEX_STRING orig_loc,
> - const char*, bool);
> + const char*, bool,
> + const char*);
> Restore_info* prepare_for_restore(String *location,
> LEX_STRING orig_loc,
> - const char*, bool);
> + const char*, bool,
> + const char*);
[3] Please add parameter names.
[4] Please do not add trailing space to new lines.
...
> --- a/sql/backup/logger.h 2009-02-13 13:25:43 +0000
> +++ b/sql/backup/logger.h 2009-03-15 19:57:33 +0000
> @@ -67,6 +67,7 @@ public:
> void report_master_binlog_pos(const st_bstream_binlog_pos&);
> void report_driver(const char *driver);
> void report_backup_file(char * path);
> + void report_comment(const char * str);
[5] Do not put space after '*' when '*' introduces a pointer.
...
> +/**
> + Report comment from user.
> +*/
[9] Insufficient function comment.
> +inline
> +void Logger::report_comment(const char *str)
> +{
> + DBUG_ASSERT(m_state == RUNNING);
> + if (!str)
> + backup_log->comment(str);
> + else
> + backup_log->comment(0);
> +}
[6] Reversed condition.
[10] You don't need the differentiation anyway. You can just
backup_log->comment(str) unconditionally.
...
> --- a/sql/si_logs.cc 2008-10-30 17:53:24 +0000
> +++ b/sql/si_logs.cc 2009-03-15 19:57:33 +0000
> @@ -88,6 +88,21 @@ void Backup_log::backup_file(const char
> }
>
> /**
> + Report the comment entered on the command line.
> +
> + This method updates the comment information in the history data.
> +
> + @param[IN] char * str The comment from the command line.
[8] We do not have parameter types in function comments.
> +*/
> +void Backup_log::comment(const char *str)
> +{
> + if (!str)
> + m_op_hist.comment= (char *)str;
> + else
> + m_op_hist.comment= 0;
> +}
[6] Reversed condition.
[10] You don't need the differentiation anyway. You can just
backup_log->comment(str) unconditionally.
...
> --- a/sql/si_logs.h 2008-11-14 14:49:09 +0000
> +++ b/sql/si_logs.h 2009-03-15 19:57:33 +0000
...
> @@ -146,6 +147,7 @@ public:
> void vp_time(time_t when, bool report);
> void add_driver(const char* driver);
> void backup_file(const char *full_path);
> + void comment(const char* str);
[5] Do not put space after '*' when '*' introduces a pointer.
...
> --- a/sql/sql_yacc.yy 2009-03-09 14:00:03 +0000
> +++ b/sql/sql_yacc.yy 2009-03-15 19:57:33 +0000
...
> @@ -6644,7 +6644,7 @@ restore:
> }
> FROM
> TEXT_STRING_sys
> - opt_overwrite opt_skip_gap_event
> + opt_overwrite opt_skip_gap_event opt_backup_comment
> {
> Lex->backup_dir = $4;
> }
[7] So you still want all options in a fixed order.
> @@ -6702,7 +6702,7 @@ backup:
> database_list
> TO_SYM
> TEXT_STRING_sys
> - opt_compression
> + opt_compression opt_backup_comment
> {
> Lex->backup_dir = $6;
> }
[7] So you still want all options in a fixed order.
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
