Hi Svoj,
Thanks for your comments. Replying below.
> The patch is not ready for push. Please see some comments
> inline. Also I'd suggest to ask Gluh to take a look at it - I
> can't say much about sys_var related changes.
Ok.
> On Tue, Jul 08, 2008 at 03:09:17PM +0200, Chuck Bell wrote:
> > === 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-08 13:08:27 +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 )
> > {
> >
> I wonder why do we need this change as backupdir is
> `path_myddir' by default.
Because when you run MTR, the path is changed for --datadir. So we must also
set --backupdir.
>
> > === 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-08 13:08:27 +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
> What is the purpose of this change? Unknown arguments are
> transparently passed to mysqld. Besides this change is not correct:
> - it overwrites DATADIR variable;
> - it always adds backupdir argument, whereas it should be
> done only if it
> was specified.
This is needed to be able to launch mysqld via mysqld_safe which could
specify a different datadir path than what the code is precompiled with. Why
is it overwriting datadir?
>
> > {
> > int res= 0;
> > + String path;
> > + int path_len= 0;
> >
> > DBUG_ENTER("execute_backup_command");
> > DBUG_ASSERT(thd && lex);
> > @@ -134,13 +140,32 @@ 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))
> This looks wrong. I think you should use test_if_hard_path() here.
Ok. I will try that. What is the difference?
>
> > + {
> > + path_len=backupdir->length() + lex->backup_dir.length + 1;
> > + path.alloc(path_len);
> > + fn_format(path.c_ptr(), lex->backup_dir.str,
> backupdir->c_ptr(), "",
> > + MY_UNPACK_FILENAME);
> > + }
> > + else
> > + {
> > + path_len= lex->backup_dir.length + 1;
> > + path.alloc(path_len);
> > + path.set_ascii(lex->backup_dir.str, lex->backup_dir.length);
> Just curious: why set_ascii()?
To ensure we don't clobber special characters.
>
> > + }
> > + path.length(path_len);
> > +
> Where do you free path?
This has been changed and now we use m_path variable of Stream class.
>
> > 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;
> THD seem to be unused.
It is removed.
> > + /*
> > + 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;
> > + }
> I think it is way to strict. mysqld can operate with wrong
> backupdir and it can even do BACKUP/RESTORE with absolute
> path. A warning should be sufficient here.
We set it to fail as per our original decision if the 'to' is invalid. If
you disagree, I recommend we bring it up as a topic in a future discussion
but leave it as a failure for this patch.
Chuck
