Merge lp:~longbow/percona-xtrabackup/fix_687544 into lp:percona-xtrabackup/2.0

[Valentine Gostev]

Added fix for bug 687544.
Merge form 1.6 - lp:~longbow/percona-xtrabackup/fix687544-1.6

[Alexey Kopytov (akopytov)]

  - the bug is targeted to 1.6, so it should be fixed in 1.6 first,
    then merged to 1.7 with a separate MP
  - :seekable is not being used, so why is it being imported
    into the global namespace?
  - there's no need to get tmpdir explicitly, File::Temp uses it by
    default according to its documentation
  - there's no need to specify the template explicitly, we don't care
    about the name, let File::Temp choose it
  - we don't care about the suffix either
  - File::Temp->new() returns a file handle, but its result is assigned
    to a variable with a file name?
    Even though the returned object is evaluated as a file name when
    used as a string, I think we should play safe and use something
    like this:

    my $mysql_stderr_handle = File::Temp->new();
    my $mysql_stderr = $mysql_stderr_handle->filename();

  - note that $mysql_stderr and $mysql_stdout are reassigned in
    init(). I think we should remove those reassignments, but please
    double check.
  - how about a test case? (it would immediately reveal the last issue,
    for example)

review: Needs Fixing

[Valentine Gostev (longbow)]

- the bug is targeted to 1.6...

Agree

- :seekable is not being used...

Unfortunately docs do not clarify why it is needed, besides that
it implements some IO::Seekable methods which may be needed in some cases.
So I verified that it works without it and will comment out.
The only reason to add this was following an example of using OO interface
from perldoc.

- there's no need to get tmpdir explicitly...

No. We need this, since by default new temp file is created in $cwd
Please check this example:

my $temp_dir=File::Spec->tmpdir();
print "Temp dir is $temp_dir \n";
my $tempfile=File::Temp->new( TEMPLATE => 'tempXXXXX',
            SUFFIX => '.pxb',
            UNLINK => 0);
my $tempfile2=File::Temp->new( TEMPLATE => 'tempXXXXX',
            DIR => $temp_dir,
            SUFFIX => '.pxb',
            UNLINK => 0);
print "File name is $tempfile \n";
print "File name is $tempfile2 \n";

[longbow@mil-x percona]$ ./test.pl
Temp dir is /tmp
File name is tempHklkd.pxb
File name is /tmp/tempReYtO.pxb
[longbow@mil-x percona]$ ls tempHklkd.pxb
tempHklkd.pxb
[longbow@mil-x percona]$ ls /tmp/tempR*
/tmp/tempReYtO.pxb
[longbow@mil-x percona]$

tmpdir() allows to find first writeable temp directory, and it is cross-platform.

- there's no need to specify the template explicitly...
- we don't care about the suffix either

1. looks nice
2. suffix allows to trace leftovers (just in case)

- File::Temp->new() returns a file handle...
That is why I use it, since it is safe to work with fh.
Keeping filename separately is a possible source of race conditions,
for example when object is being destroyed, but variable keeps old
filename.
Since object acts both as filename and fh - it does not make sense to keep filename separately.

- note that $mysql_stderr and $mysql_stdout...
Yes, they are reassigned but they are responsible for another leftovers (/tmp/mysql_std[err,out] after stream backup)
I think it is ok to use the same fh.

- how about a test case?
testing that stderr and stdout are not there would not reveal above issue, since those are different leftovers kept in different location. But test will require to add debug option to verify that files are created correctly and removed after backup finished.
So I guess right now it is not worth to do it

[Valentine Gostev (longbow)]

Alexey,

and thanks for your comment :)
I will prepare MP for 1.6

[Alexey Kopytov (akopytov)]

On 24.11.11 12:51, Valentine Gostev wrote:
> - there's no need to get tmpdir explicitly...
>
> No. We need this, since by default new temp file is created in $cwd
> Please check this example:
>
> my $temp_dir=File::Spec->tmpdir();
> print "Temp dir is $temp_dir \n";
> my $tempfile=File::Temp->new( TEMPLATE => 'tempXXXXX',
> SUFFIX => '.pxb',
> UNLINK => 0);
> my $tempfile2=File::Temp->new( TEMPLATE => 'tempXXXXX',
> DIR => $temp_dir,
> SUFFIX => '.pxb',
> UNLINK => 0);
> print "File name is $tempfile \n";
> print "File name is $tempfile2 \n";
>
> [longbow@mil-x percona]$ ./test.pl
> Temp dir is /tmp
> File name is tempHklkd.pxb
> File name is /tmp/tempReYtO.pxb
> [longbow@mil-x percona]$ ls tempHklkd.pxb
> tempHklkd.pxb
> [longbow@mil-x percona]$ ls /tmp/tempR*
> /tmp/tempReYtO.pxb
> [longbow@mil-x percona]$
>
> tmpdir() allows to find first writeable temp directory, and it is cross-platform.
>

Right, and it is used by File::Temp. The reason it is created in the
current directory in the first case is that you specify template without
specifying a directory explicitly. Try removing TEMPLATE from your examples.

> - there's no need to specify the template explicitly...
> - we don't care about the suffix either
>
> 1. looks nice

Come on, how much nicer is tempHtwm_.pxb as compared to NARg17D6OP.pxb
or just yjOxKMXEGD?

> 2. suffix allows to trace leftovers (just in case)
>

There can be no leftovers, those files are removed automatically unless
UNLINK => 0 is used. Yes, even if innobackupex fails with an error or
crashes. Yes, on Windows too.

> - File::Temp->new() returns a file handle...
> That is why I use it, since it is safe to work with fh.
> Keeping filename separately is a possible source of race conditions,
> for example when object is being destroyed, but variable keeps old
> filename.
> Since object acts both as filename and fh - it does not make sense to keep filename separately.
>

So you've RTFMed which is good, but you didn't understand it which is bad.

File::Temp returns both a filename and an open file handle to guarantee
the temporary file is not replaced between getting the filename,
checking that it doesn't exist and opening it.

Our case is different. If you take a look at how $mysql_stdout and
$mysql_stderr are actually used, you'll see that we don't use (and thus,
don't need) the filehandle at all. We just the filename in shell
commands. So those security considerations from the manual are simply
not applicable: all we need is a temporary file name, and there's always
a chance it will be overwritten/replaced between different mysql client
invocations.

Which actually means all we need is tmpnam(), File::Temp->new() is an
overkill for our purposes.

> - note that $mysql_stderr and $mysql_stdout...
> Yes, they are reassigned but they are responsible for another leftovers (/tmp/mysql_std[err,out] after stream backup)
> I think it is ok to use the same fh.
>

It will _not_ use the same fh, as no fh is actually being used. It will
assign "$backup_dir/mysql-stderr" or "$option_tmpdir/mysql-stderr" and
use those _file_names_.

> - how about a test case?
>...

[Alexey Kopytov (akopytov)]

Changing to Rejected, as it has to be resubmitted for 1.6 rather than trunk.

review: Needs Resubmitting

[Alexey Kopytov (akopytov)]

review: Approve

[Valentine Gostev (longbow)]

http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-param/92/

=== modified file 'innobackupex'
--- innobackupex	2011-11-29 07:22:31 +0000
+++ innobackupex	2011-11-30 08:49:24 +0000
@@ -12,9 +12,10 @@
 use Pod::Usage qw(pod2usage);
 use POSIX "strftime";
 use POSIX ":sys_wait_h";
-use POSIX "tmpnam";
+# use POSIX "tmpnam";
 use FileHandle;
 use File::Basename;
+use File::Temp;
 use English qw(-no_match_vars);
 
 # version of this script
@@ -152,10 +153,12 @@
 my $mysql_server_version = '';
 
 # name of the file where stderr of mysql process is directed
-my $mysql_stderr = 'stderr';
+my $mysql_stderr_fh = File::Temp->new();
+my $mysql_stderr = $mysql_stderr_fh->filename;
 
 # name of the file where stdout of mysql process is directed
-my $mysql_stdout = 'stdout';
+my $mysql_stdout_fh = File::Temp->new();
+my $mysql_stdout = $mysql_stdout_fh->filename;
 
 # name of the file where binlog position info is written
 my $binlog_info;
@@ -1495,15 +1498,11 @@
         if (!$option_remote_host && !$option_stream) {
         $backup_config_file = $backup_dir . '/backup-my.cnf';
         $suspend_file = $backup_dir . '/xtrabackup_suspended';
-        $mysql_stdout = $backup_dir . '/mysql-stdout';
-        $mysql_stderr = $backup_dir . '/mysql-stderr';
         $binlog_info = $backup_dir . '/xtrabackup_binlog_info';
         $slave_info = $backup_dir . '/xtrabackup_slave_info';
         } else {
         $suspend_file = get_option(\%config, 'mysqld', 'datadir') . '/xtrabackup_suspended';
         $tmp_logfile = $option_tmpdir . '/xtrabackup_logfile';
-        $mysql_stdout = $option_tmpdir . '/mysql-stdout';
-        $mysql_stderr = $option_tmpdir . '/mysql-stderr';
         if ($option_stream) {
             $backup_config_file = $option_tmpdir . '/backup-my.cnf';
             $binlog_info = $option_tmpdir . '/xtrabackup_binlog_info';