STATUS
------
Not (yet) approved.
REQUIRED
--------
1. Mask backup_id in the test.
2. Fix comments as Ingo suggested: make sentences start with capital letter
and end with dot. This is even specified in coding guidelines (see
http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines#Commenting_Code):
"* Each standalone comment must start with a Capital letter.
* There is a '.' at the end of each statement in a comment paragraph (for
the last one as well)."
SUGGESTIONS
-----------
3. Change logic in Output_stream::open().
4. Issue warning if existing file is overwritten.
COMMENTARY
----------
The patch is in general OK. Only the mentioned minor things need to be fixed
before it is good for pushing. I reinforced Ingo's suggestion about comments
as requirement, because it is explicitly required by our coding guidelines.
Also because I'm working on fixing guideline violations in our code and I
prefer that you fix them here rather than I fix them later ;)
DETAILS
-------
Thava Alagu wrote:
> #At file:///home/thava/repo/bkup/ based on
> revid:oystein.grovlen@stripped
>
> 2881 Thava Alagu 2009-10-15
> BUG#36402 - Backup needs option to overwrite backup image
>
> The backup sql command now takes additional option "OVERWRITE"
> to overwrite backup image file even if one exists with the same name.
>
> modified:
> mysql-test/suite/backup/r/backup_errors.result
> mysql-test/suite/backup/t/backup_errors.test
> sql/backup/backup_kernel.h
> sql/backup/kernel.cc
> sql/backup/stream.cc
> sql/backup/stream.h
> sql/sql_parse.cc
> sql/sql_yacc.yy
...
> === modified file 'mysql-test/suite/backup/t/backup_errors.test'
> --- a/mysql-test/suite/backup/t/backup_errors.test 2009-03-10 10:02:06 +0000
> +++ b/mysql-test/suite/backup/t/backup_errors.test 2009-10-15 01:48:34 +0000
> @@ -90,6 +90,11 @@ eval BACKUP DATABASE adb TO "test.bak" $
> --replace_regex /Errcode: [0-9]+/Errcode: #/
> SHOW WARNINGS;
>
> +--echo # overwrite existing backup image if overwrite option specified
> +eval BACKUP DATABASE adb TO "test.bak" $compression OVERWRITE;
[1] You must mask the backup_id value as it will vary in different runs of
the test leading to different results and test failure. We mask output of
BACKUP by saying "--replace_column 1 #" before BACKUP invocation.
> +# Overwriting backup image should succeed now.
> +SHOW WARNINGS;
> +
> --echo verify backup history and progress logs for backup_state.
> SELECT backup_state,operation, backup_file, command FROM mysql.backup_history;
> SELECT notes FROM mysql.backup_progress;
>
...
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc 2009-09-21 11:39:02 +0000
> +++ b/sql/backup/kernel.cc 2009-10-15 01:48:34 +0000
> @@ -163,8 +163,8 @@ static int send_reply(Backup_restore_ctx
> @param[in] thd current thread object reference.
> @param[in] lex results of parsing the statement.
> @param[in] backupdir value of the backupdir variable from server.
> - @param[in] overwrite whether or not restore should overwrite existing
> - DB with same name as in backup image
> + @param[in] overwrite For restore: should overwrite DB with same name.
> + For backup: should overwrite backup image file.
> @param[in] skip_gap_event whether or not restore should skip writing
> the gap event if run on a master in an active
> replication scenario
Nice that you updated documentation accordingly :)
> @@ -213,7 +213,8 @@ execute_backup_command(THD *thd,
> DEBUG_SYNC(thd, "before_backup_prepare");
> Backup_info *info= context.prepare_for_backup(backupdir, lex->backup_dir,
> thd->query,
> - lex->backup_compression);
> + lex->backup_compression,
> + overwrite);
> // reports errors
>
> // Error condition insertion for ER_BACKUP_BACKUP_PREPARE.
> @@ -730,7 +731,8 @@ Backup_info*
> Backup_restore_ctx::prepare_for_backup(String *backupdir,
> LEX_STRING orig_loc,
> const char *query,
> - bool with_compression)
> + bool with_compression,
> + bool overwrite)
> {
> using namespace backup;
>
> @@ -771,7 +773,8 @@ Backup_restore_ctx::prepare_for_backup(S
> DEBUG_SYNC(m_thd, "before_backup_open_stream");
> Output_stream *s= new Output_stream(*this,
> &m_path,
> - with_compression);
> + with_compression,
> + overwrite);
> m_stream= s;
>
> if (!s || is_killed())
>
> === modified file 'sql/backup/stream.cc'
> --- a/sql/backup/stream.cc 2009-09-21 11:39:02 +0000
> +++ b/sql/backup/stream.cc 2009-10-15 01:48:34 +0000
> @@ -425,10 +425,11 @@ int Stream::close()
>
>
> Output_stream::Output_stream(Logger &log, ::String *path,
> - bool with_compression)
> + bool with_compression, bool overwrite)
> :Stream(log, path, 0)
> {
> m_with_compression= with_compression;
> + m_overwrite= overwrite;
> stream.write= stream_write;
> m_block_size=0; // use default block size provided by the backup stram library
> }
> @@ -517,10 +518,16 @@ int Output_stream::open()
> MY_STAT stat_info;
> close(); // If close() should fail, we will still try to open
>
> - /* Allow to write to existing named pipe */
> - if (my_stat(m_path->c_ptr(), &stat_info, MYF(0)) &&
> - MY_S_ISFIFO(stat_info.st_mode))
> - m_flags= O_WRONLY;
> + /* Allow overwriting if named pipe or if overwrite option specified */
> + if (my_stat(m_path->c_ptr(), &stat_info, MYF(0)))
> + {
> + if (MY_S_ISFIFO(stat_info.st_mode))
> + m_flags= O_WRONLY;
> + else if (m_overwrite)
> + m_flags= O_WRONLY|O_TRUNC;
> + else
> + m_flags= O_WRONLY|O_CREAT|O_EXCL|O_TRUNC;
> + }
> else
> m_flags= O_WRONLY|O_CREAT|O_EXCL|O_TRUNC;
>
[3] To avoid code repetition, I suggest changing the logic like this:
m_flags= O_WRONLY|O_CREAT|O_EXCL|O_TRUNC;
if (my_stat(m_path->c_ptr(), &stat_info, MYF(0)))
{
if (MY_S_ISFIFO(stat_info.st_mode))
m_flags= O_WRONLY;
else if (m_overwrite)
m_flags= O_WRONLY|O_TRUNC;
}
[4] I think it would be good to issue a warning if we overwrite existing file.
>
> === modified file 'sql/backup/stream.h'
> --- a/sql/backup/stream.h 2009-08-21 15:43:19 +0000
> +++ b/sql/backup/stream.h 2009-10-15 01:48:34 +0000
> @@ -122,7 +122,7 @@ class Output_stream: public Stream
> public:
>
> /// Constructor
> - Output_stream(Logger&, ::String *, bool);
> + Output_stream(Logger&, ::String *, bool, bool);
>
> int open();
> int close();
> @@ -136,6 +136,7 @@ private:
>
> int write_magic_and_version();
> bool init();
> + bool m_overwrite;
> };
>
> /// Used to read from backup stream.
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2009-10-06 12:07:32 +0000
> +++ b/sql/sql_parse.cc 2009-10-15 01:48:34 +0000
> @@ -2401,8 +2401,11 @@ mysql_execute_command(THD *thd)
> sys_var_backupdir.value_length);
> backupdir.length(sys_var_backupdir.value_length);
>
> - /* Used to specify if RESTORE should overwrite existing db with same name */
> - bool overwrite_restore= false;
> + /*
> + overwrite for RESTORE overwrites existing db with same name.
> + overwrite for BACKUP overwrites existing backup image.
> + */
[2] Start with capital letter etc.
> + bool overwrite_option= false;
>
> /* Used to specify if RESTORE should skup writing the gap event. */
> bool skip_gap_event= false;
...
Rafal
