On Wed, Apr 15, 2015 at 02:59:55PM +0900, Michael Paquier wrote:
> On Wed, Apr 15, 2015 at 2:38 PM, Noah Misch wrote:
> > Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8. Therefore,
> > Perl
> > installations lacking this File::Path feature will receive vendor support
> > for
> > years to come. Replacing the use of keep_root with rmtree+mkdir will add
> > 2-10
> > lines of code, right? Let's do that and not raise the system requirements.
>
> Good point. Let's use mkdir in combination then. Attached is an updated patch.
> --- a/src/bin/initdb/t/001_initdb.pl
> +++ b/src/bin/initdb/t/001_initdb.pl
> @@ -1,6 +1,7 @@
> use strict;
> use warnings;
> use TestLib;
> +use File::Path qw(rmtree);
> use Test::More tests => 19;
>
> my $tempdir = TestLib::tempdir;
> @@ -18,27 +19,30 @@ command_fails([ 'initdb', '-S', "$tempdir/data3" ],
> mkdir "$tempdir/data4" or BAIL_OUT($!);
> command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory');
>
> -system_or_bail "rm -rf '$tempdir'/*";
> -
> +rmtree($tempdir);
> +mkdir $tempdir;
It's usually wrong to remove and recreate the very directory made by
File::Temp. Doing so permits a race condition: an attacker can replace the
directory between the rmdir() and the mkdir(). However, TestLib::tempdir
returns a subdirectory of the build directory, and the build directory is
presumed secure. That's good enough.
> -system_or_bail "rm -rf '$tempdir'/*";
> +rmtree($tempdir);
> command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ],
> 'select default dictionary');
You omitted the mkdir() on that last one. It works, since initdb does the
equivalent of "mkdir -p", but it looks like an oversight.
As I pondered this, I felt it would do better to solve a different problem.
The "rm -rf" invocations presumably crept in to reduce peak disk usage.
Considering the relatively-high duration of a successful initdb run, I doubt
we get good value from so many positive tests. I squashed those positive
tests into one, as attached. (I changed the order of negative tests but kept
them all.) This removed the need for intra-script cleaning, and it
accelerated script runtime from 22s to 9s. Does this seem good to you?
Thanks,
nm
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index d12be84..ed13bdc 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -1,44 +1,32 @@
use strict;
use warnings;
use TestLib;
-use Test::More tests => 19;
+use Test::More tests => 14;
my $tempdir = TestLib::tempdir;
+my $xlogdir = "$tempdir/pgxlog";
+my $datadir = "$tempdir/data";
program_help_ok('initdb');
program_version_ok('initdb');
program_options_handling_ok('initdb');
-command_ok([ 'initdb', "$tempdir/data" ], 'basic initdb');
-command_fails([ 'initdb', "$tempdir/data" ], 'existing data directory');
-command_ok([ 'initdb', '-N', "$tempdir/data2" ], 'nosync');
-command_ok([ 'initdb', '-S', "$tempdir/data2" ], 'sync only');
-command_fails([ 'initdb', '-S', "$tempdir/data3" ],
+command_fails([ 'initdb', '-S', "$tempdir/nonexistent" ],
'sync missing data directory');
-mkdir "$tempdir/data4" or BAIL_OUT($!);
-command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory');
-system_or_bail "rm -rf '$tempdir'/*";
-
-command_ok([ 'initdb', '-X', "$tempdir/pgxlog", "$tempdir/data" ],
- 'separate xlog directory');
-
-system_or_bail "rm -rf '$tempdir'/*";
+mkdir $xlogdir;
+mkdir "$xlogdir/lost+found";
+command_fails(
+ [ 'initdb', '-X', $xlogdir, $datadir ],
+ 'existing nonempty xlog directory');
+rmdir "$xlogdir/lost+found";
command_fails(
- [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ],
+ [ 'initdb', $datadir, '-X', 'pgxlog' ],
'relative xlog directory not allowed');
-system_or_bail "rm -rf '$tempdir'/*";
-mkdir "$tempdir/pgxlog";
-command_ok([ 'initdb', '-X', "$tempdir/pgxlog", "$tempdir/data" ],
- 'existing empty xlog directory');
-
-system_or_bail "rm -rf '$tempdir'/*";
-mkdir "$tempdir/pgxlog";
-mkdir "$tempdir/pgxlog/lost+found";
-command_fails([ 'initdb', '-X', "$tempdir/pgxlog", "$tempdir/data" ],
- 'existing nonempty xlog directory');
+mkdir $datadir;
+command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
+ 'successful creation');
-system_or_bail "rm -rf '$tempdir'/*";
-command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ],
- 'select default dictionary');
+command_ok([ 'initdb', '-S', $datadir ], 'sync only');
+command_fails([ 'initdb', $datadir ], 'existing data directory');
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
