Make more portable TAP tests of initdb

Michael Paquier wrote:

Hi all,

I noticed that src/bin/initdb/t/001_initdb.pl uses directly rm via a
system() call like that:
system_or_bail "rm -rf '$tempdir'/*";

This way of doing is not portable, particularly on platforms that do
not have rm like... Windows where the equivalent is del. And we could
actually use remove_tree with its option keep_root to get the same
effect in pure perl as mentioned here:
http://perldoc.perl.org/File/Path.html
With this formulation:
remove_tree($tempdir, {keep_root => 1});

Does Perl 5.8 have this?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Apr 14, 2015, at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

http://perldoc.perl.org/File/Path.html
With this formulation:
remove_tree($tempdir, {keep_root => 1});

Does Perl 5.8 have this?

Yes, it does.

http://cpansearch.perl.org/src/NWCLARK/perl-5.8.9/lib/File/Path.pm

Best,

David

On Tue, Apr 14, 2015 at 09:25:33AM -0700, David E. Wheeler wrote:

On Apr 14, 2015, at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

http://perldoc.perl.org/File/Path.html
With this formulation:
remove_tree($tempdir, {keep_root => 1});

Does Perl 5.8 have this?

Yes, it does.

http://cpansearch.perl.org/src/NWCLARK/perl-5.8.9/lib/File/Path.pm

One way to avoid a lot of these questions would be to use
Test::MinimumVersion or similar in a git hook. If we're promising
5.8.mumble, we should be able to keep that promise always rather than
search case by case, at least up to bugs in the version-checking
software.

http://search.cpan.org/~rjbs/Test-MinimumVersion-0.101081/lib/Test/MinimumVersion.pm

What say?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David E. Wheeler wrote:

On Apr 14, 2015, at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

http://perldoc.perl.org/File/Path.html
With this formulation:
remove_tree($tempdir, {keep_root => 1});

Does Perl 5.8 have this?

Yes, it does.

http://cpansearch.perl.org/src/NWCLARK/perl-5.8.9/lib/File/Path.pm

Uh. 5.8.9 does, but 5.8.8 does not. Not sure if this is a problem.
Ancient buildfarm member pademelon (Tom's HPUX stuff) has 5.8.9 -- but
that was probably installed by hand. Brolga (really ancient Cygwin
stuff) has 5.10.1. As far as I can tell (perldoc perldelta5101) it was
upgraded to File::Path 2.07_03 which should be fine. Spoonbill has
5.12.2; 5.12.0 already included the fixes in 5.10.1 so it should be fine
also. Friarbird has 5.12.4.

Castoroides has 5.8.4. Oops.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Castoroides has 5.8.4. Oops.

WUT.

David E. Wheeler wrote:

On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Castoroides has 5.8.4. Oops.

WUT.

Yeah, eh? Anyway I don't think it matters much: just don't enable TAP
tests on machines with obsolete Perl. I think this is fine since 5.8's
latest release is supported.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Apr 15, 2015 at 5:29 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

David E. Wheeler wrote:

On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Castoroides has 5.8.4. Oops.

WUT.

Yeah, eh? Anyway I don't think it matters much: just don't enable TAP
tests on machines with obsolete Perl. I think this is fine since 5.8's
latest release is supported.

And castoroides is not using --enable-tap-tests in the buildfarm btw.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Apr 14, 2015 at 05:29:36PM -0300, Alvaro Herrera wrote:

David E. Wheeler wrote:

On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Castoroides has 5.8.4. Oops.

WUT.

Yeah, eh? Anyway I don't think it matters much: just don't enable TAP
tests on machines with obsolete Perl. I think this is fine since 5.8's
latest release is supported.

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.

Thanks,
nm

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.
--
Michael

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

On Thu, Apr 30, 2015 at 12:17 PM, Noah Misch <noah@leadboat.com> wrote:

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.

OK, I haven't thought of that.

-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?

That's a fight code simplicity vs. test performance. FWIW, I like the
test suite with many positive tests because it is easy to read the
test flow. And as this test suite is meant to grow up with new tests
in the future, I am guessing that people may not follow the
restriction this patch adds all the time.

 command_fails(
-       [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ],
+       [ 'initdb', $datadir, '-X', 'pgxlog' ],
        'relative xlog directory not allowed');
$datadir should be the last argument. This would break on platforms
where src/port/getopt_long.c is used, like Windows.

+command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
+       'successful creation');
This is grouping the test for nosync, existing nonempty xlog
directory, and the one for selection of the default dictionary.
Shouldn't this test name be updated otherwise then?

Could you add a comment mentioning that tests are grouped to reduce
I/O impact during a run?
Regards,
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, May 01, 2015 at 03:23:44PM +0900, Michael Paquier wrote:

On Thu, Apr 30, 2015 at 12:17 PM, Noah Misch <noah@leadboat.com> wrote:

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?

That's a fight code simplicity vs. test performance. FWIW, I like the
test suite with many positive tests because it is easy to read the
test flow. And as this test suite is meant to grow up with new tests
in the future, I am guessing that people may not follow the
restriction this patch adds all the time.

True.

command_fails(
-       [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ],
+       [ 'initdb', $datadir, '-X', 'pgxlog' ],
'relative xlog directory not allowed');
$datadir should be the last argument. This would break on platforms
where src/port/getopt_long.c is used, like Windows.

I committed that as a separate patch; thanks.

+command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
+       'successful creation');
This is grouping the test for nosync, existing nonempty xlog
directory, and the one for selection of the default dictionary.
Shouldn't this test name be updated otherwise then?

Could you add a comment mentioning that tests are grouped to reduce
I/O impact during a run?

Committed with a comment added. The "successful creation" test name is
generic because it's the designated place to add testing of new options.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, May 3, 2015 at 8:09 AM, Noah Misch <noah@leadboat.com> wrote:

On Fri, May 01, 2015 at 03:23:44PM +0900, Michael Paquier wrote:

On Thu, Apr 30, 2015 at 12:17 PM, Noah Misch <noah@leadboat.com> wrote:

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?

That's a fight code simplicity vs. test performance. FWIW, I like the
test suite with many positive tests because it is easy to read the
test flow. And as this test suite is meant to grow up with new tests
in the future, I am guessing that people may not follow the
restriction this patch adds all the time.

True.

command_fails(
-       [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ],
+       [ 'initdb', $datadir, '-X', 'pgxlog' ],
'relative xlog directory not allowed');
$datadir should be the last argument. This would break on platforms
where src/port/getopt_long.c is used, like Windows.

I committed that as a separate patch; thanks.

+command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
+       'successful creation');
This is grouping the test for nosync, existing nonempty xlog
directory, and the one for selection of the default dictionary.
Shouldn't this test name be updated otherwise then?

Could you add a comment mentioning that tests are grouped to reduce
I/O impact during a run?

Committed with a comment added. The "successful creation" test name is
generic because it's the designated place to add testing of new options.

Thanks. The overall result looks good to me.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers