Re: bzr commit into mysql-6.0-backup branch (cbell:2648) Bug#35230

[Rafal Somla]

Chuck,

I still don't like the changes in the backup kernel. The serious concern is that 
  you report an error always, if backupdir is invalid, but I think error should 
be reported only if backupdir used. If user says something like

BACKUP DATABASE foo TO '/an/absolute/path';

I think it should succeed even if backupdir is currently invalid, because the 
command does not refer to backupdir.

The easiest solution IMO would be to just warn about invalid backupdir. If it is 
really used, user will get error when the stream is opened. I'm aware that it is 
not perfect because error message could be confusing, but I think we can deal 
with that separately, as I think this particular place needs to be refactored 
anyway (i.e., reporting stream related errors).

Apart from that there are few changes which I don't think are needed and thus 
are a bit annoying for an author of the code. These are things like changing 
types of function parameters, renaming the parameters and such. I'd like to see 
the reasons behind introducing such changes...

See below for more details.

Rafal

Chuck Bell wrote:
> #At file:///D:/source/bzr/mysql-6.0-bug-35230/
> 
>  2648 Chuck Bell	2008-07-09
>       BUG#35230 Backup: no backupdir 
>       
>       This patch adds the dynamic variable --backupdir which specifies the path
>       where the image files are placed (backup) or accessed (restore) if a path
>       is not specified in the command.
> modified:
>   mysql-test/r/backup_progress.result
>   mysql-test/t/backup_progress.test
>   sql/backup/backup_kernel.h
>   sql/backup/kernel.cc
>   sql/backup/stream.cc
>   sql/backup/stream.h
>   sql/mysqld.cc
>   sql/set_var.cc
>   sql/set_var.h
>   sql/share/errmsg.txt
>   sql/sql_parse.cc
> 
> per-file messages:
>   mysql-test/r/backup_progress.result
>     New result file.
>   mysql-test/t/backup_progress.test
>     Added a mask for backup location because this path will vary from 
>     machine to machine.
>   sql/backup/backup_kernel.h
>     Changed methods to include original location from lex structure instead of
>     traversing the THD object.
>   sql/backup/kernel.cc
>     Added code to process the backupdir IFF there is no path specified 
>     in the command.
>   sql/backup/stream.cc
>     Moved path processing to Stream::Stream. Constructors now take backupdir plus
>     original filename specified on command.
>   sql/backup/stream.h
>     Constructors now take backupdir plus original filename specified on command.
>   sql/mysqld.cc
>     Added code to support backupdir.
>   sql/set_var.cc
>     Added all of the methods needed to check, update, and set a default 
>     for the backupdir variable.
>   sql/set_var.h
>     Added extern for backupdir variable named sys_var_backupdir.
>   sql/share/errmsg.txt
>     Added new error message for invalid backupdir paths.
>   sql/sql_parse.cc
>     Added processing of system variable for backupdir to remove dependency in
>     backup code.
> === modified file 'mysql-test/r/backup_progress.result'
> --- a/mysql-test/r/backup_progress.result	2008-07-01 20:32:27 +0000
> +++ b/mysql-test/r/backup_progress.result	2008-07-09 19:08:25 +0000
> @@ -78,7 +78,7 @@ start_time	#
>  stop_time	#
>  host_or_server_name	localhost
>  username	root
> -backup_file	backup_progress_orig.bak
> +backup_file	#
>  user_comment	
>  command	BACKUP DATABASE backup_progress to 'backup_progress_orig.bak'
>  engines	MyISAM, Default, Snapshot
> @@ -135,7 +135,7 @@ start_time	#
>  stop_time	#
>  host_or_server_name	localhost
>  username	root
> -backup_file	backup_progress_orig.bak
> +backup_file	#
>  user_comment	
>  command	RESTORE FROM 'backup_progress_orig.bak'
>  engines	MyISAM, Default, Snapshot
> 
> === modified file 'mysql-test/t/backup_progress.test'
> --- a/mysql-test/t/backup_progress.test	2008-06-25 13:39:04 +0000
> +++ b/mysql-test/t/backup_progress.test	2008-07-09 19:08:25 +0000
> @@ -107,7 +107,7 @@ connection con2;
>  reap;
>  
>  #Show results
> ---replace_column 1 # 2 # 3 # 4 # 10 # 11 # 12 #
> +--replace_column 1 # 2 # 3 # 4 # 10 # 11 # 12 # 15 #
>  --query_vertical SELECT ob.* FROM mysql.online_backup AS ob JOIN
> backup_progress.t1_res AS t1 ON ob.backup_id = t1.id;
>  --replace_column 1 # 3 # 4 #
>  SELECT obp.* FROM mysql.online_backup_progress AS obp JOIN backup_progress.t1_res AS
> t1 ON obp.backup_id = t1.id;
> @@ -156,7 +156,7 @@ reap;
>  DELETE FROM backup_progress.t1_res;
>  SELECT MAX(backup_id) INTO @bup_id FROM mysql.online_backup WHERE command LIKE
> "RESTORE FROM%";
>  INSERT INTO backup_progress.t1_res (id) VALUES (@bup_id);
> ---replace_column 1 # 2 # 3 # 4 # 10 # 11 # 12 #
> +--replace_column 1 # 2 # 3 # 4 # 10 # 11 # 12 # 15 #
>  --query_vertical SELECT ob.* FROM mysql.online_backup AS ob JOIN
> backup_progress.t1_res AS t1 ON ob.backup_id = t1.id;
>  --replace_column 1 # 3 # 4 #
>  SELECT obp.* FROM mysql.online_backup_progress AS obp JOIN backup_progress.t1_res AS
> t1 ON obp.backup_id = t1.id;
> 
> === modified file 'sql/backup/backup_kernel.h'
> --- a/sql/backup/backup_kernel.h	2008-06-26 12:10:46 +0000
> +++ b/sql/backup/backup_kernel.h	2008-07-09 19:08:25 +0000
> @@ -30,7 +30,7 @@ void backup_shutdown();
>    Called from the big switch in mysql_execute_command() to execute
>    backup related statement
>  */
> -int execute_backup_command(THD*, LEX*);
> +int execute_backup_command(THD*, LEX*, String*);
>  
>  // forward declarations
>  
> @@ -66,8 +66,12 @@ class Backup_restore_ctx: public backup:
>    bool is_valid() const;
>    ulonglong op_id() const;
>  
> -  Backup_info*  prepare_for_backup(LEX_STRING location, const char*, bool);
> -  Restore_info* prepare_for_restore(LEX_STRING location, const char*);  
> +  Backup_info*  prepare_for_backup(String *location, 
> +                                   LEX_STRING orig_loc, 
> +                                   const char*, bool);
> +  Restore_info* prepare_for_restore(String *location, 
> +                                   LEX_STRING orig_loc,
> +                                   const char*);  
>  

Why parameter which holds value of backupdir is called location, not backupdir? 
Then location could be preserved as a parameter holding, well... the location.

>    int do_backup();
>    int do_restore();
> 
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc	2008-07-01 13:34:36 +0000
> +++ b/sql/backup/kernel.cc	2008-07-09 19:08:25 +0000
> @@ -19,7 +19,8 @@
>    {
>    
>     Backup_restore_ctx context(thd); // create context instance
> -   Backup_info *info= context.prepare_for_backup(location); // prepare for backup
> +   Backup_info *info= context.prepare_for_backup(location, 
> +                                                 orig_loc); // prepare for backup
>    
>     // select objects to backup
>     info->add_all_dbs();
> @@ -41,7 +42,8 @@
>    {
>    
>     Backup_restore_ctx context(thd); // create context instance
> -   Restore_info *info= context.prepare_for_restore(location); // prepare for
> restore
> +   Restore_info *info= context.prepare_for_restore(location,
> +                                                   orig_loc); // prepare for
> restore
>    
>     context.do_restore(); // perform restore
>     
> @@ -111,7 +113,9 @@ static int send_reply(Backup_restore_ctx
>  /**
>    Call backup kernel API to execute backup related SQL statement.
>  
> -  @param lex  results of parsing the statement.
> +  @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.
>  
>    @note This function sends response to the client (ok, result set or error).
>  
> @@ -119,7 +123,7 @@ static int send_reply(Backup_restore_ctx
>   */
>  
>  int
> -execute_backup_command(THD *thd, LEX *lex)
> +execute_backup_command(THD *thd, LEX *lex, String *backupdir)
>  {
>    int res= 0;
>    
> @@ -127,6 +131,7 @@ execute_backup_command(THD *thd, LEX *le
>    DBUG_ASSERT(thd && lex);
>    DEBUG_SYNC(thd, "before_backup_command");
>  
> +    
>    using namespace backup;
>  
>    Backup_restore_ctx context(thd); // reports errors
> @@ -134,13 +139,24 @@ execute_backup_command(THD *thd, LEX *le
>    if (!context.is_valid())
>      DBUG_RETURN(send_error(context, ER_BACKUP_CONTEXT_CREATE));
>  
> +  /*
> +    Check backupdir for validity (needed since we cannot trust
> +    that the path has become invalid since set).
> +  */
> +  if (backupdir->length() && my_access(backupdir->c_ptr(),
> (F_OK|W_OK)))
> +  {
> +    context.fatal_error(ER_BACKUP_BACKUPDIR, backupdir->c_ptr());
> +    DBUG_RETURN(send_error(context, ER_BACKUP_BACKUPDIR, backupdir->c_ptr()));
> +  }
> +

I think it is too strict to always report error here. I would report warning 
instead and the error will happen when openning stream, but not if user 
specifies absolute path.

>    switch (lex->sql_command) {
>  
>    case SQLCOM_BACKUP:
>    {
>      // prepare for backup operation
>      
> -    Backup_info *info= context.prepare_for_backup(lex->backup_dir,
> thd->query,
> +    Backup_info *info= context.prepare_for_backup(backupdir, lex->backup_dir, 
> +                                                  thd->query,
>                                                    lex->backup_compression);
>                                                                // reports errors
>  
> @@ -185,7 +201,8 @@ execute_backup_command(THD *thd, LEX *le
>  
>    case SQLCOM_RESTORE:
>    {
> -    Restore_info *info= context.prepare_for_restore(lex->backup_dir,
> thd->query);
> +    Restore_info *info= context.prepare_for_restore(backupdir, lex->backup_dir, 
> +                                                    thd->query);
>      
>      if (!info || !info->is_valid())
>        DBUG_RETURN(send_error(context, ER_BACKUP_RESTORE_PREPARE));
> @@ -396,6 +413,11 @@ int Backup_restore_ctx::prepare(LEX_STRI
>  
>    // check if location is valid (we assume it is a file path)
>  
> +  /*
> +    For this error to work correctly, we need to check original
> +    file specified by the user rather than the path formed
> +    using the backupdir.
> +  */
>    bool bad_filename= (location.length == 0);
>    
>    /*
> @@ -405,7 +427,7 @@ int Backup_restore_ctx::prepare(LEX_STRI
>  #if defined(__WIN__) || defined(__EMX__)  
>  
>    bad_filename = bad_filename || check_if_legal_filename(location.str);
> -  
> +
>  #endif
>  
>    if (bad_filename)
> @@ -443,7 +465,8 @@ int Backup_restore_ctx::prepare(LEX_STRI
>  /**
>    Prepare for backup operation.
>    
> -  @param[in] location   path to the file where backup image should be stored
> +  @param[in] backupdir  path to the file where backup image should be stored
> +  @param[in] orig_loc   path as specified on command line for backup image
>    @param[in] query      BACKUP query starting the operation
>    @param[in] with_compression  backup image compression switch
>    
> @@ -458,7 +481,9 @@ int Backup_restore_ctx::prepare(LEX_STRI
>    is performed using @c do_backup() method.
>   */ 
>  Backup_info* 
> -Backup_restore_ctx::prepare_for_backup(LEX_STRING location, const char *query,
> +Backup_restore_ctx::prepare_for_backup(String *backupdir, 
> +                                       LEX_STRING orig_loc, 
> +                                       const char *query,
>                                         bool with_compression)

Why renaming location -> orig_loc?

>  {
>    using namespace backup;
> @@ -466,7 +491,7 @@ Backup_restore_ctx::prepare_for_backup(L
>    if (m_error)
>      return NULL;
>    
> -  if (Logger::init(BACKUP, location, query))
> +  if (Logger::init(BACKUP, orig_loc, query))
>    {
>      fatal_error(ER_BACKUP_LOGGER_INIT);
>      return NULL;
> @@ -479,16 +504,16 @@ Backup_restore_ctx::prepare_for_backup(L
>      Do preparations common to backup and restore operations. After call
>      to prepare() all meta-data changes are blocked.
>     */ 
> -  if (prepare(location))
> +  if (prepare(orig_loc))
>      return NULL;
>  
> -  backup::String path(location);

This variable was here for transforming the string from the representation used 
in the parser (LEX_STRING) to the representation used in the backup kernel code 
(String).

> -  
>    /*
>      Open output stream.
>     */
> -
> -  Output_stream *s= new Output_stream(*this, path, with_compression);
> +  Output_stream *s= new Output_stream(*this, 
> +                                      backupdir, 
> +                                      orig_loc,
> +                                      with_compression);
>    m_stream= s;
>    
>    if (!s)
> @@ -499,7 +524,11 @@ Backup_restore_ctx::prepare_for_backup(L
>    
>    if (!s->open())
>    {
> -    fatal_error(ER_BACKUP_WRITE_LOC, path.ptr());
> +    /*
> +      For this error, use the actual value returned instead of the
> +      path complimented with backupdir.
> +    */
> +    fatal_error(ER_BACKUP_WRITE_LOC, orig_loc.str);
>      return NULL;
>    }
>  
> @@ -528,7 +557,8 @@ Backup_restore_ctx::prepare_for_backup(L
>  /**
>    Prepare for restore operation.
>    
> -  @param[in] location   path to the file where backup image is stored
> +  @param[in] backupdir  path to the file where backup image is stored
> +  @param[in] orig_loc   path as specified on command line for backup image
>    @param[in] query      RESTORE query starting the operation
>    
>    @returns Pointer to a @c Restore_info instance containing catalogue of the
> @@ -537,14 +567,16 @@ Backup_restore_ctx::prepare_for_backup(L
>    @note This function reports errors.
>   */ 
>  Restore_info* 
> -Backup_restore_ctx::prepare_for_restore(LEX_STRING location, const char *query)
> +Backup_restore_ctx::prepare_for_restore(String *backupdir,
> +                                        LEX_STRING orig_loc, 
> +                                        const char *query)

Why renaming location -> orig_loc?

>  {
>    using namespace backup;  
>  
>    if (m_error)
>      return NULL;
>    
> -  if (Logger::init(RESTORE, location, query))
> +  if (Logger::init(RESTORE, orig_loc, query))
>    {
>      fatal_error(ER_BACKUP_LOGGER_INIT);
>      return NULL;
> @@ -557,15 +589,14 @@ Backup_restore_ctx::prepare_for_restore(
>      Do preparations common to backup and restore operations. After this call
>      changes of meta-data are blocked.
>     */ 
> -  if (prepare(location))
> +  if (prepare(orig_loc))
>      return NULL;
>    
>    /*
>      Open input stream.
>     */
>  
> -  backup::String path(location);

This variable was here for transforming the string from the representation used 
in parser (LEX_STRING) to the representation used in the backup kernel code 
(String).

> -  Input_stream *s= new Input_stream(*this, path);
> +  Input_stream *s= new Input_stream(*this, backupdir, orig_loc);
>    m_stream= s;
>    
>    if (!s)
> @@ -576,7 +607,11 @@ Backup_restore_ctx::prepare_for_restore(
>    
>    if (!s->open())
>    {
> -    fatal_error(ER_BACKUP_READ_LOC, path.ptr());
> +    /*
> +      For this error, use the actual value returned instead of the
> +      path complimented with backupdir.
> +    */
> +    fatal_error(ER_BACKUP_READ_LOC, orig_loc.str);
>      return NULL;
>    }
>  
> 
> === modified file 'sql/backup/stream.cc'
> --- a/sql/backup/stream.cc	2008-07-02 11:27:17 +0000
> +++ b/sql/backup/stream.cc	2008-07-09 19:08:25 +0000
> @@ -190,9 +190,30 @@ extern "C" int stream_read(void *instanc
>  }
>  
>  
> -Stream::Stream(Logger &log, const ::String &name, int flags)
> -  :m_path(name), m_flags(flags), m_block_size(0), m_log(log)
> +Stream::Stream(Logger &log, ::String *backupdir, 
> +               LEX_STRING orig_loc, int flags)
> +  :m_flags(flags), m_block_size(0), m_log(log)

Why "const ::String&" is replaced with "::String*" ?
Why renaming name -> orig_loc?
Why changing its type to LEX_STRING?

I'd prefer to consistently use String for passing string values in the backup 
code. There was one exception - the parameter of execute_backup_command() was of 
type LEX_STRING. But this is an exception because the function is called from 
inside  the parser which uses LEX_STRING internaly. Inside backup (kernel) code 
I wanted to use String class for strings.

>  {
> +  int path_len= 0;
> +
> +  /*
> +    Prepare the path using the backupdir iff no path included
> +  */
> +  if (!test_if_hard_path(orig_loc.str))
> +  {
> +    path_len=backupdir->length() + orig_loc.length + 1;
> +    m_path.alloc(path_len);
> +    fn_format(m_path.c_ptr(), orig_loc.str, backupdir->c_ptr(), "",
> +              MY_UNPACK_FILENAME);
> +  }
> +  else
> +  {
> +    path_len= orig_loc.length + 1;
> +    m_path.alloc(path_len);

Why do you allocate memory here?

> +    m_path.set_ascii(orig_loc.str, orig_loc.length);
> +  }
> +  m_path.length(path_len);
> +

I think m_path.set_ascii(...) sets the length of the string, so this 
m_patch.length(...) statement is needed only inside if(...) branch, where 
fn_format(...) is used.

>    bzero(&stream, sizeof(stream));
>    bzero(&buf, sizeof(buf));
>    bzero(&mem, sizeof(mem));
> @@ -228,9 +249,10 @@ bool Stream::rewind()
>  }
>  
>  
> -Output_stream::Output_stream(Logger &log, const ::String &name,
> +Output_stream::Output_stream(Logger &log, ::String *backupdir,
> +                             LEX_STRING orig_loc,
>                               bool with_compression)
> -  :Stream(log, name, 0)
> +  :Stream(log, backupdir, orig_loc, 0)
>  {
>    m_with_compression= with_compression;
>    stream.write= stream_write;
> @@ -414,8 +436,9 @@ bool Output_stream::rewind()
>  }
>  
>  
> -Input_stream::Input_stream(Logger &log, const ::String &name)
> -  :Stream(log, name, O_RDONLY)
> +Input_stream::Input_stream(Logger &log, ::String *backupdir, 
> +                           LEX_STRING orig_loc)
> +  :Stream(log, backupdir, orig_loc, O_RDONLY)
>  {
>    m_with_compression= false;
>    stream.read= stream_read;
>
(cut)
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2008-06-25 13:39:04 +0000
> +++ b/sql/sql_parse.cc	2008-07-09 19:08:25 +0000
> @@ -38,7 +38,7 @@
>    @defgroup Runtime_Environment Runtime Environment
>    @{
>  */
> -int execute_backup_command(THD*,LEX*);
> +int execute_backup_command(THD*, LEX*, String*);
>  
>  /* Used in error handling only */
>  #define SP_TYPE_STRING(LP) \
> @@ -2168,10 +2168,21 @@ mysql_execute_command(THD *thd)
>  #else
>    {
>      /*
> +      Create a string from the backupdir system variable and pass
> +      to backup system.
> +    */
> +    String backupdir;
> +
> +    backupdir.alloc(sys_var_backupdir.value_length);
> +    backupdir.set_ascii(sys_var_backupdir.value, 
> +                        sys_var_backupdir.value_length);
> +    backupdir.length(sys_var_backupdir.value_length);
> +

Do you really need to copy the value of the variable? Why not just pass the 
pointer to the value?

> +    /*
>        Note: execute_backup_command() sends a correct response to the client
>        (either ok, result set or error message).
> -     */  
> -    if (execute_backup_command(thd,lex))
> +     */ 
> +    if (execute_backup_command(thd, lex, &backupdir))

Since parser uses LEX_STRING for holdings strings, it would be more appropriate 
to pass backupdir as LEX_STRING. Even if we need to copy the value of backupdir, 
this can be done inside execute_backup_command.

>        goto error;
>      break;
>    }
> 
>