Hi Chuck,
I request some changes to the patch before it can be pushed. These are related
to unnecessary memory allocations and string copying in the code.
Another issue I raise is the fact that you refer to thd->lex->backup_dir in the
code. This creates another unwanted dependency between backup code and the rest
of the server, as I explain below. I propose a solution for that, which is
consequently using the "raw" name of the backup file, as specified by the user,
and adding the backupdir to it only inside the backup::Stream class, where the
corresponding file must be opened and a full path to it must be known.
As we discussed before, an ultimate implementation of --backupdir and other
backup related options would require designing a general mechanism for passing
options from server to the online backup module. However, we need --backupdir
option now and we don't have any such mechanism at hand. So I accept pushing
this code as a temporary solution (given that other concerns are resolved),
keeping in mind that we would have to refactor it when general backup options
handling mechanism is developed.
See more detailed comments below.
Chuck Bell wrote:
> #At file:///D:/source/bzr/mysql-6.0-bug-35230/
>
> 2648 Chuck Bell 2008-07-02
> 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.
>
> The default is the same as --datadir.
> modified:
> include/config-netware.h
> include/config-win.h
> mysql-test/mysql-test-run.pl
> mysql-test/r/backup_progress.result
> mysql-test/t/backup_progress.test
> scripts/mysqld_safe.sh
> sql/backup/kernel.cc
> sql/mysqld.cc
> sql/set_var.cc
> sql/set_var.h
> sql/share/errmsg.txt
> sql/unireg.h
>
> per-file messages:
> include/config-netware.h
> Added #define for backupdir.
> include/config-win.h
> Added #define for backupdir.
> mysql-test/mysql-test-run.pl
> Added backupdir to MTR to properly set the default path for test runs.
> 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.
> scripts/mysqld_safe.sh
> Added setting of backupdir for launching on Linux systems.
> sql/backup/kernel.cc
> Added code to process the backupdir IFF there is no path specified in the
> 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/unireg.h
> Added #define for backupdir.
> === modified file 'include/config-netware.h'
> --- a/include/config-netware.h 2007-07-23 21:54:55 +0000
> +++ b/include/config-netware.h 2008-07-02 19:37:44 +0000
> @@ -132,6 +132,7 @@ extern "C" {
> #define SHAREDIR "share/"
> #define DEFAULT_CHARSET_HOME "sys:/mysql/"
> #define DATADIR "data/"
> +#define BACKUPDIR "data/"
>
> /* 64-bit file system calls */
> #define SIZEOF_OFF_T 8
>
> === modified file 'include/config-win.h'
> --- a/include/config-win.h 2008-07-02 11:27:17 +0000
> +++ b/include/config-win.h 2008-07-02 19:37:44 +0000
> @@ -341,6 +341,7 @@ inline double ulonglong2double(ulonglong
> #else
> #define DEFAULT_MYSQL_HOME "c:\\mysql"
> #define DATADIR "c:\\mysql\\data"
> +#define BACKUPDIR "c:\\mysql\\data"
> #define PACKAGE "mysql"
> #define DEFAULT_BASEDIR "C:\\"
> #define SHAREDIR "share"
>
> === modified file 'mysql-test/mysql-test-run.pl'
> --- a/mysql-test/mysql-test-run.pl 2008-07-02 07:53:34 +0000
> +++ b/mysql-test/mysql-test-run.pl 2008-07-02 19:37:44 +0000
> @@ -3663,6 +3663,17 @@ sub mysqld_arguments ($$$$) {
> mtr_add_arg($args, "%s--datadir=%s", $prefix,
> $mysqld->{'path_myddir'});
>
> + #
> + # Add the --backupdir parameter for running backup tests from MTR.
> + # Note: In the future, this setting should be moved to an option
> + # file located in the backup suite directory. The file should contain
> + # the following and be named 'suite.opt':
> + #
> + # --backupdir=$MYSQLTEST_VARDIR/tmp
> + #
> + mtr_add_arg($args, "%s--backupdir=%s", $prefix,
> + $mysqld->{'path_myddir'});
> +
>
> if ( $mysql_version_id >= 50106 )
> {
>
> === 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-02 19:37:44 +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-02 19:37:44 +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 'scripts/mysqld_safe.sh'
> --- a/scripts/mysqld_safe.sh 2008-03-11 16:15:47 +0000
> +++ b/scripts/mysqld_safe.sh 2008-07-02 19:37:44 +0000
> @@ -156,6 +156,7 @@ parse_arguments() {
> # these get passed explicitly to mysqld
> --basedir=*) MY_BASEDIR_VERSION="$val" ;;
> --datadir=*) DATADIR="$val" ;;
> + --backupdir=*) DATADIR="$val" ;;
> --pid-file=*) pid_file="$val" ;;
> --user=*) user="$val"; SET_USER=1 ;;
>
> @@ -534,7 +535,7 @@ fi
> cmd="$NOHUP_NICENESS"
>
> for i in "$ledir/$MYSQLD" "$defaults" "--basedir=$MY_BASEDIR_VERSION" \
> - "--datadir=$DATADIR" "$USER_OPTION"
> + "--datadir=$DATADIR" "--backupdir=$DATADIR" "$USER_OPTION"
> do
> cmd="$cmd "`shell_quote_string "$i"`
> done
>
> === 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-02 19:37:44 +0000
> @@ -122,6 +122,8 @@ int
> execute_backup_command(THD *thd, LEX *lex)
> {
> int res= 0;
> + LEX_STRING path;
> + int path_len= 0;
It would be more convenient and safer to use the String class here. You could do
that like this:
String path;
...
path.alloc(sys_var_backupdir.value_length + lex->backup_dir.length + 1);
fn_format(path.c_ptr(),...);
...
path.set_ascii(lex->backup_dir.str, lex->backup_dir.length);
...
This is safer because memory is freed automatically if it was allocated.
>
> DBUG_ENTER("execute_backup_command");
> DBUG_ASSERT(thd && lex);
> @@ -134,13 +136,31 @@ execute_backup_command(THD *thd, LEX *le
> if (!context.is_valid())
> DBUG_RETURN(send_error(context, ER_BACKUP_CONTEXT_CREATE));
>
> + /*
> + Prepare the path using the backupdir iff no path included
> + */
> + if (!has_path(lex->backup_dir.str))
> + {
> + path.length= sys_var_backupdir.value_length + lex->backup_dir.length + 1;
> + path.str= (char *)my_malloc(path.length + 1, MYF(0));
> + strcpy(path.str, fn_format(path.str, lex->backup_dir.str,
> + sys_var_backupdir.value, "",
> + MY_UNPACK_FILENAME));
The fn_format() function copies the result to its first argument - the strcpy is
not needed.
Referring to global sys_var_backupdir variable creates an unwanted implicit
dependency between backup kernel and the server.
> + }
> + else
> + {
> + path.length= lex->backup_dir.length + 1;
> + path.str= (char *)my_malloc(path.length + 1, MYF(0));
> + strcpy(path.str, lex->backup_dir.str);
> + }
Why do you allocate new memory and copy the string? lex->backup_dir.str will not
disappear while execute_backup_command() is running.
> +
> 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(path, thd->query,
> lex->backup_compression);
> // reports errors
>
> @@ -185,7 +205,7 @@ 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(path, thd->query);
>
> if (!info || !info->is_valid())
> DBUG_RETURN(send_error(context, ER_BACKUP_RESTORE_PREPARE));
> @@ -209,6 +229,7 @@ execute_backup_command(THD *thd, LEX *le
>
> } // switch(lex->sql_command)
>
> + my_free(path.str, MYF(0)); // free memory for path
Are you sure this my_free() will be always executed - even if some errors are
detected above?
> if (context.close())
> DBUG_RETURN(send_error(context, ER_BACKUP_CONTEXT_REMOVE));
>
> @@ -396,7 +417,12 @@ int Backup_restore_ctx::prepare(LEX_STRI
>
> // check if location is valid (we assume it is a file path)
>
> - bool bad_filename= (location.length == 0);
> + /*
> + 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= (m_thd->lex->backup_dir.length == 0);
This kind of code creates yet another obscure dependency between backup code and
the server. Namely, to compile this code, we must know the internal structure of
THD object, hence we must include the header defining it and then we must
basically include all of the server code because of the internal
dependencies there.
This type of code will make it virtually impossible to separate backup tree from
the server tree. If we hope to be ever able to do it, we must never use THD
directly, but treat THD* as pointers to opaque objects which we sometimes must
pass to server functions but we never try to guess what is inside them.
Note that in mysql_execute_command() I have a separate LEX parameter exactly to
avoid looking into the THD one!
An alternative I propose here is to pass to Backup_restore_ctx::prepare() the
"raw" location given by user in BACKUP/RESTORE command, as it was done before.
The default path can be added only when needed, which is at the time of opening
the stream I think.
>
> /*
> On some systems certain file names are invalid. We use
> @@ -404,13 +430,13 @@ int Backup_restore_ctx::prepare(LEX_STRI
> */
> #if defined(__WIN__) || defined(__EMX__)
>
> - bad_filename = bad_filename || check_if_legal_filename(location.str);
> -
> + bad_filename = bad_filename ||
> check_if_legal_filename(m_thd->lex->backup_dir.str);
> +
> #endif
>
> if (bad_filename)
> {
> - fatal_error(ER_BAD_PATH, location.str);
> + fatal_error(ER_BAD_PATH, m_thd->lex->backup_dir.str);
> return m_error;
> }
>
> @@ -499,7 +525,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;
> + fatal_error(ER_BACKUP_WRITE_LOC, thd->lex->backup_dir.str);
> return NULL;
> }
>
One way of thinking about the backupdir setting is that it changes semantics of
the backup locations specified by users in BACKUP/RESTORE commands. Thus
'test.bak' doesn't mean "file test.bak in the current working directory" but it
rather means "file test.bak in the default backup directory". Consequently, when
one creates Output_stream object passing "test.bak" as the name of the file, it
should be understood as "open a stream for writing to file test.bak in the
default backup directory". Thus, the backupdir path should be added to the name
inside Output_stream constructor which must translate name 'test.bak' from the
backup semantics (i.e., "file test.bak in the default backup directory") to the
system wide semantics used by low level functions (i.e., "file
/path/do/default/backup/dir/test.bak")
Thus I suggest to do the path transformation inside constructor of the
backup::Stream class (so that it will be shared by Input/Output_stream.
> @@ -576,7 +607,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;
> + fatal_error(ER_BACKUP_READ_LOC, thd->lex->backup_dir.str);
> return NULL;
> }
>
>
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc 2008-07-01 20:32:27 +0000
> +++ b/sql/mysqld.cc 2008-07-02 19:37:44 +0000
> @@ -1362,6 +1362,7 @@ void clean_up(bool print_message)
> my_free(sys_init_slave.value, MYF(MY_ALLOW_ZERO_PTR));
> my_free(sys_var_general_log_path.value, MYF(MY_ALLOW_ZERO_PTR));
> my_free(sys_var_slow_log_path.value, MYF(MY_ALLOW_ZERO_PTR));
> + my_free(sys_var_backupdir.value, MYF(MY_ALLOW_ZERO_PTR));
> free_tmpdir(&mysql_tmpdir_list);
> #ifdef HAVE_REPLICATION
> my_free(slave_load_tmpdir,MYF(MY_ALLOW_ZERO_PTR));
> @@ -5745,6 +5746,8 @@ struct my_option my_long_options[] =
> "Creating and dropping stored procedures alters ACLs. Disable with
> --skip-automatic-sp-privileges.",
> (uchar**) &sp_automatic_privileges, (uchar**) &sp_automatic_privileges,
> 0, GET_BOOL, NO_ARG, 1, 0, 0, 0, 0, 0},
> + {"backupdir", 'B', "Path used to store backup data.", (uchar**)
> &sys_var_backupdir.value,
> + (uchar**) &sys_var_backupdir.value, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0,
> 0},
> {"basedir", 'b',
> "Path to installation directory. All paths are usually resolved relative to
> this.",
> (uchar**) &mysql_home_ptr, (uchar**) &mysql_home_ptr, 0, GET_STR,
> REQUIRED_ARG,
> @@ -7853,6 +7856,22 @@ mysqld_get_one_option(int optid,
> case 'l':
> opt_log=1;
> break;
> + case 'B':
> + {
> + /*
> + Check if directory exists and we have permission to create file and
> + write to file.
> + */
> + if (my_access(argument, (F_OK|W_OK)))
> + {
> + fprintf(stderr, "The path specified for the variable backupdir cannot"
> + " be accessed or is invalid. ref:'%s'\n", argument);
> + exit(1);
> + }
> + strcpy(sys_var_backupdir.value, argument);
> + sys_var_backupdir.value_length= strlen(sys_var_backupdir.value);
> + break;
> + }
> case 'h':
> strmake(mysql_real_data_home,argument, sizeof(mysql_real_data_home)-1);
> /* Correct pointer set by my_getopt (for embedded library) */
>
> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc 2008-05-27 19:47:15 +0000
> +++ b/sql/set_var.cc 2008-07-02 19:37:44 +0000
> @@ -149,6 +149,9 @@ static bool sys_update_general_log_path(
> static void sys_default_general_log_path(THD *thd, enum_var_type type);
> static bool sys_update_slow_log_path(THD *thd, set_var * var);
> static void sys_default_slow_log_path(THD *thd, enum_var_type type);
> +static int sys_check_backupdir(THD *thd, set_var *var);
> +static bool sys_update_backupdir(THD *thd, set_var * var);
> +static void sys_default_backupdir(THD *thd, enum_var_type type);
>
> /*
> Variable definition list
> @@ -778,6 +781,15 @@ sys_var_str sys_var_slow_log_path(&vars,
> opt_slow_logname);
> static sys_var_log_output sys_var_log_output_state(&vars, "log_output",
> &log_output_options,
> &log_output_typelib, 0);
> +
> +/*
> + Create the backupdir dynamic variable.
> +*/
> +sys_var_str sys_var_backupdir(&vars, "backupdir",
> + sys_check_backupdir,
> + sys_update_backupdir,
> + sys_default_backupdir,
> + 0);
>
>
> /*
> @@ -2614,6 +2626,142 @@ uchar *sys_var_log_output::value_ptr(THD
> return (uchar*) thd->strmake(tmp.ptr(), length);
> }
>
> +/*
> + Functions for backupdir variable.
> +*/
> +
In a more modular design, these functions could be provided by the online backup
module. The server will only ensure that they are called when the variable is
accessed.
That is, this would be a design where server still needs to be aware that there
is a variable called backupdir and that it belongs to the online backup module.
Even more modular design would be such that modules, when activated, inform
server about new variables/options they support and server will extend the set
of available ones dynamically. But that is perhaps too ambitious...
Rafal
