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

[Rafal Somla]

Chuck,

I'm satisfied with changes wrt memory allocation and strings. Also, nice trick 
for isolating access to the server variable from the backup code. I have one 
question below about a thd variable which is not used anywhere I think.

When I run the code, I got such warnings from MTR (found in var/log/warnings):

master.err: main.backup: Error: Freeing wrong aligned pointer at line 1365, 
'mysqld.cc'

The line is where sys_var_backupdir.value is freed. Can you see this also in 
your tree? If yes then it needs to be fixed.


About the place where the backupdir is appended to the backup location. You wrote:

> (...) I could not move the
> path manipulation code to the constructor of the stream object (base)
> because the path is accessed prior to the stream creation point from
> prepare().

I don't think that you really need to access the full path anywhere except when 
opening the file. See the attached patch which implements the idea I described. 
I hope you agree to use that code in your patch (note that it makes the changes 
in the backup kernel much smaller).


Chuck Bell wrote:
> @@ -499,7 +533,12 @@ 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.
> +    */
> +    THD *thd= ::current_thd;

I wonder why this thd variable is defined here?

> +    fatal_error(ER_BACKUP_WRITE_LOC, orig_loc.str);
>      return NULL;
>    }
>

> @@ -576,7 +617,12 @@ 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.
> +    */
> +    THD *thd= ::current_thd;

I wonder why this thd variable is defined here?

> +    fatal_error(ER_BACKUP_READ_LOC, orig_loc.str);
>      return NULL;
>    }
>  

Rafal

=== modified file 'sql/backup/backup_kernel.h'
--- sql/backup/backup_kernel.h	2008-07-07 12:51:56 +0000
+++ sql/backup/backup_kernel.h	2008-07-09 10:57:43 +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
 

=== modified file 'sql/backup/kernel.cc'
--- sql/backup/kernel.cc	2008-07-08 19:58:46 +0000
+++ sql/backup/kernel.cc	2008-07-09 10:57:57 +0000
@@ -76,6 +76,11 @@
 #include "ddl_blocker.h"
 #include "backup_progress.h"
 
+namespace backup {
+
+String opt_backupdir; ///< The default directory where backup files are created.
+
+} // backup namespace
 
 /** 
   Global Initialization for online backup system.
@@ -111,7 +116,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 +126,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;
   
@@ -129,6 +136,14 @@ execute_backup_command(THD *thd, LEX *le
 
   using namespace backup;
 
+  // Save the value of backupdir option.
+  
+  if(backupdir)
+    opt_backupdir= *backupdir;
+  else
+    //  A hard coded default in case the server doesn't provide one.
+    opt_backupdir.set_ascii("data/",5);
+
   Backup_restore_ctx context(thd); // reports errors
   
   if (!context.is_valid())

=== modified file 'sql/backup/stream.cc'
--- sql/backup/stream.cc	2008-07-02 11:27:17 +0000
+++ sql/backup/stream.cc	2008-07-09 11:19:56 +0000
@@ -21,6 +21,8 @@ const unsigned char backup_magic_bytes[8
 };
 
 namespace backup {
+        
+extern String opt_backupdir;
 
 /**
   Low level write for backup stream library.
@@ -191,7 +193,7 @@ 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)
+  :m_flags(flags), m_block_size(0), m_log(log)
 {
   bzero(&stream, sizeof(stream));
   bzero(&buf, sizeof(buf));
@@ -199,6 +201,19 @@ Stream::Stream(Logger &log, const ::Stri
   bzero(&data_buf, sizeof(data_buf));
   block_size= 0;
   state= CLOSED;
+
+  /*
+    Prepare the path using the backupdir iff no path included
+  */
+  if (!has_path(name.ptr()))
+  {
+    m_path.alloc(opt_backupdir.length() + name.length() + 2);
+    fn_format(m_path.c_ptr(), name.ptr(), opt_backupdir.ptr(), "",
+              MY_UNPACK_FILENAME);
+    m_path.length(strlen(m_path.ptr()));
+  }
+  else
+    m_path= name;
 }
 
 bool Stream::open()