TAP / recovery-test fs-level backups, psql enhancements etc

On Tue, Mar 1, 2016 at 2:48 PM, Craig Ringer <craig@2ndquadrant.com> wrote:

Hi all

I've been working with the new TAP tests for recovery and have a number of
enhancements I'd like to make to the tooling to make writing tests easier
and nicer. I've also included two improvements proposed by Kyotaro
HORIGUCHI in the prior thread about the now-committed TAP recovery tests.

I developed these changes as part of testing failover slots and logical
decoding timeline following, where I found a need for better control over
psql, the ability to make filesystem level backups, etc. It doesn't make
sense to try to jam all that into my test script when it belongs in the
infrastructure.

Patches attached, each explains what it does and what for.

You are using both "die_on_error" and "on_error_die" in your code. That
looks like a mismatch!

On 1 March 2016 at 22:08, salvador fandino <sfandino@gmail.com> wrote:

On Tue, Mar 1, 2016 at 2:48 PM, Craig Ringer <craig@2ndquadrant.com>
wrote:

Hi all

I've been working with the new TAP tests for recovery and have a number
of enhancements I'd like to make to the tooling to make writing tests
easier and nicer. I've also included two improvements proposed by Kyotaro
HORIGUCHI in the prior thread about the now-committed TAP recovery tests.

I developed these changes as part of testing failover slots and logical
decoding timeline following, where I found a need for better control over
psql, the ability to make filesystem level backups, etc. It doesn't make
sense to try to jam all that into my test script when it belongs in the
infrastructure.

Patches attached, each explains what it does and what for.

You are using both "die_on_error" and "on_error_die" in your code. That
looks like a mismatch!

Thanks for taking a look.

In this case it has no effect since it's just specifying the default
explicitly, but it's certainly wrong. Fixed in git.

I'll wait to see what else comes up before posting another series.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer wrote:

Hi all

I've been working with the new TAP tests for recovery and have a number of
enhancements I'd like to make to the tooling to make writing tests easier
and nicer.

I think we should change the existing psql method to be what you propose
as psql_expert. I don't see any advantage in keeping the old one. Many
of the existing uses of psql should become what you call psql_check; but
we should probably call that psql_ok() instead, and also have a
psql_fails() for src/test/recovery/t/001_stream_rep.pl (and others to
come).

+=item $node->backup_fs_hot(backup_name)
+
+Create a backup with a filesystem level copy in $node->backup_dir,
+including transaction logs. Archiving must be enabled as pg_start_backup
+and pg_stop_backup are used. This is not checked or enforced.

Perhaps "WAL archiving or streaming must be enabled ..."

I would rather have the patches be submitted with a reasonable
approximation at indentation rather than submit a separate indent patch.

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

Just pushed 0006.

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

Craig Ringer wrote:

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 3d11cbb..8c13655 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -112,9 +112,11 @@ INIT
#
sub tempdir
{
+	my ($prefix) = @_;
+	$prefix = "tmp_test" if (!$prefix);

This should be "unless defined $prefix". Otherwise if you pass the
string literal "0" as prefix it will be ignored.

--
�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 2 March 2016 at 07:07, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Craig Ringer wrote:

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 3d11cbb..8c13655 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -112,9 +112,11 @@ INIT
#
sub tempdir
{
+     my ($prefix) = @_;
+     $prefix = "tmp_test" if (!$prefix);

This should be "unless defined $prefix". Otherwise if you pass the
string literal "0" as prefix it will be ignored.

Ha. I thought something was funny with !$prefix when splitting that out of
Kyotaro Horiguchi's patch but couldn't put my finger on what.

Will amend in the next push. I'll keep the committed 006 Retain tempdirs
for failed tests in that series but amend it to show it's committed
upstream.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2 March 2016 at 05:46, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I think we should change the existing psql method to be what you propose
as psql_expert. I don't see any advantage in keeping the old one. Many
of the existing uses of psql should become what you call psql_check; but
we should probably call that psql_ok() instead, and also have a
psql_fails() for src/test/recovery/t/001_stream_rep.pl (and others to
come).

I agree and that's what I really wanted to do. I just didn't want to
produce a massive diff that renames the method across all of src/bin etc
too, since I thought that'd be harder to commit and might have backporting
consequences.

If you think that's the way to go I'm very happy with that and will proceed.

+=item $node->backup_fs_hot(backup_name)
+
+Create a backup with a filesystem level copy in $node->backup_dir,
+including transaction logs. Archiving must be enabled as pg_start_backup
+and pg_stop_backup are used. This is not checked or enforced.

Perhaps "WAL archiving or streaming must be enabled ..."

Good point, will do.

I would rather have the patches be submitted with a reasonable
approximation at indentation rather than submit a separate indent patch.

The reason I didn't do that is that the indenting in PostgresNode.pm is
already well out of whack and, TBH, I didn't want to rebase on top of a
perltidy'd version. I can bite the bullet and move the perltidy to the
start of the patch series then make sure each subsequent patch is tidy'd
but I'd want to be very sure you'd be OK to commit the perltidy of
PostgresNode.pm otherwise I'd have to rebase messily all over again...

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2016-03-02 6:57 GMT+08:00 Alvaro Herrera <alvherre@2ndquadrant.com>:

Just pushed 0006.

This upset buildfarm members running prehistoric Perl versions because
is_passing was added after 5.8.8.

Fix attached.

I think I'm going to have to do an archaeology-grade Perl install, there's
just too much to keep an eye on manually. Ugh.

Why do we requite a 10yo Perl anyway?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2 March 2016 at 11:22, Craig Ringer <craig@2ndquadrant.com> wrote:

2016-03-02 6:57 GMT+08:00 Alvaro Herrera <alvherre@2ndquadrant.com>:

Just pushed 0006.

This upset buildfarm members running prehistoric Perl versions because
is_passing was added after 5.8.8.

Fix attached.

Really, this time.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2 March 2016 at 11:23, Craig Ringer <craig@2ndquadrant.com> wrote:

Really, this time.

Really, really this time, the version in git that actually works, not a
format-patch'd version before I made a last fix. Sigh. I can't even blame
lack of coffee...

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2 March 2016 at 10:07, Craig Ringer <craig@2ndquadrant.com> wrote:

On 2 March 2016 at 05:46, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I think we should change the existing psql method to be what you propose
as psql_expert. I don't see any advantage in keeping the old one. Many
of the existing uses of psql should become what you call psql_check; but
we should probably call that psql_ok() instead, and also have a
psql_fails() for src/test/recovery/t/001_stream_rep.pl (and others to
come).

I agree and that's what I really wanted to do. I just didn't want to
produce a massive diff that renames the method across all of src/bin etc
too, since I thought that'd be harder to commit and might have backporting
consequences.

If you think that's the way to go I'm very happy with that and will
proceed.

I'll make the change you suggested to make 'psql_expert' into 'psql' and
change call sites to use it or psql_check as appropriate. I'll probably
make it an immediately following patch in the series so it's easier to
separate the bulk-rename from the functional changes, but it can be
trivially squashed for commit.

On reflection I want to keep the name as psql_check, rather than psql_ok.
I'll insert another patch that changes src/bin to use psql_check where
appropriate.

The reason I used _check rather than psql_ok is partly that psql_check
isn't a test. It doesn't run any Test::More checks, it die()s on failure
because failure isn't expected but is incidental to the test that's using
psql. I did it that way because I don't think the psql invocation should be
a test in its self - then you'd have to pass a test name to every psql_ok
invocation and you'd get a flood of meaningless micro-tests showing up that
obscure the real thing being tested. It'd also be a PITA maintaining the
number of tests in the tests => 'n' argument to Test::More.

So I'm inclined to keep it as psql_check, to avoid confusion with the names
'ok' and 'fails' that Test::More uses. It's not actually a test. I don't
think we need or should have a psql_ok wrapper, since with this change you
can now just write:

is($node->psql('db', 'SELECT syntaxerror;'), 3, 'psql exits with code 3 on
syntax error');

which is clearer and more specific than:

$node->psql_ok('db', 'SELECT syntaxerror;', test => 'psql exits on syntax
error');

The reason I didn't do that is that the indenting in PostgresNode.pm is
already well out of whack and, TBH, I didn't want to rebase on top of a
perltidy'd version. I can bite the bullet and move the perltidy to the
start of the patch series then make sure each subsequent patch is tidy'd
but I'd want to be very sure you'd be OK to commit the perltidy of
PostgresNode.pm otherwise I'd have to rebase messily all over again...

This wasn't as bad as I thought. I pulled the tidy changes to the
$self->psql stuff into that patch and rebased the rest to the start of the
series so it only touches what's currently committed. I agree that's better.

Updated tree pushed. I'll send a new patch series once I've done the
psql_ok part.

It's funny that as part of implementing timeline following in logical
decoding and implementing failover slots I'm writing perl test framework
improvements....

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig@2ndquadrant.com> writes:

This upset buildfarm members running prehistoric Perl versions because
is_passing was added after 5.8.8.

Sir, RHEL6 is not prehistoric ... and this is failing on my server too.
I'm not sure when "is_passing" was added, but it was later than 5.10.1.

Fix attached.

Will check and apply.

I think I'm going to have to do an archaeology-grade Perl install, there's
just too much to keep an eye on manually. Ugh.
Why do we requite a 10yo Perl anyway?

The point of running ancient versions in the buildfarm is so that we
don't move the goalposts on software compatibility without knowing it.
When we come across a good reason to desupport an old Perl or Tcl or
whatever version, we'll do it; but having to add another ten lines or
so of code doesn't seem like a good reason.

regards, tom lane

--
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, Mar 2, 2016 at 1:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Craig Ringer <craig@2ndquadrant.com> writes:

This upset buildfarm members running prehistoric Perl versions because
is_passing was added after 5.8.8.

Sir, RHEL6 is not prehistoric ... and this is failing on my server too.
I'm not sure when "is_passing" was added, but it was later than 5.10.1.

Test::Builder is managing this variable, as documented here for people
wondering:
http://perldoc.perl.org/Test/Builder.html
I am tracking the first version as being 5.12.0.

{details} would prove to work, it is available in 5.8.8.
--
Michael

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

Craig Ringer <craig@2ndquadrant.com> writes:

Really, really this time, the version in git that actually works, not a
format-patch'd version before I made a last fix. Sigh. I can't even blame
lack of coffee...

Hmm, still doesn't work for me: make check-world dies with

Can't use string ("Test::Builder") as a HASH ref while "strict refs" in use at /
usr/share/perl5/Test/Builder.pm line 1798.
END failed--call queue aborted.
# Looks like your test exited with 22 just after 14.

The referenced line number is the end of the file, which is right in
keeping with the TAP tests' infrastructure's habit of being utterly
impenetrable when it's not working. My small amount of Perl-fu is
not sufficient to debug this.

perl-5.10.1-141.el6_7.1.x86_64
perl-Test-Harness-3.17-141.el6_7.1.x86_64
perl-Test-Simple-0.92-141.el6_7.1.x86_64

regards, tom lane

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

I wrote:

Can't use string ("Test::Builder") as a HASH ref while "strict refs" in use at /usr/share/perl5/Test/Builder.pm line 1798.

The referenced line number is the end of the file,

Oh, scratch that; I was looking at the wrong file. Actually,
/usr/share/perl5/Test/Builder.pm has

sub details {
my $self = shift;
return @{ $self->{Test_Results} };
}

and line 1798 is the "return" statement in that. I still lack enough
Perl-fu to decipher this, though.

regards, tom lane

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

Tom Lane wrote:

I wrote:

Can't use string ("Test::Builder") as a HASH ref while "strict refs" in use at /usr/share/perl5/Test/Builder.pm line 1798.

The referenced line number is the end of the file,

Oh, scratch that; I was looking at the wrong file. Actually,
/usr/share/perl5/Test/Builder.pm has

sub details {
my $self = shift;
return @{ $self->{Test_Results} };
}

and line 1798 is the "return" statement in that. I still lack enough
Perl-fu to decipher this, though.

I think it's complaining about being called as a class method. Perhaps
this would work:

+ foreach my $detail (Test::More->builder->details())

--
�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, Mar 2, 2016 at 2:18 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Tom Lane wrote:

I wrote:

Can't use string ("Test::Builder") as a HASH ref while "strict refs" in use at /usr/share/perl5/Test/Builder.pm line 1798.

The referenced line number is the end of the file,

Oh, scratch that; I was looking at the wrong file. Actually,
/usr/share/perl5/Test/Builder.pm has

sub details {
my $self = shift;
return @{ $self->{Test_Results} };
}

and line 1798 is the "return" statement in that. I still lack enough
Perl-fu to decipher this, though.

I think it's complaining about being called as a class method. Perhaps
this would work:

+ foreach my $detail (Test::More->builder->details())

Yes, that's the problem. Instead of using details(), summary() is
enough actually. And it is enough to let caller know the failure when
just one test has been found as not passing. See attached.
--
Michael

On 2 March 2016 at 13:22, Michael Paquier <michael.paquier@gmail.com> wrote:

On Wed, Mar 2, 2016 at 2:18 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Tom Lane wrote:

I wrote:

Can't use string ("Test::Builder") as a HASH ref while "strict refs"

in use at /usr/share/perl5/Test/Builder.pm line 1798.

The referenced line number is the end of the file,

Oh, scratch that; I was looking at the wrong file. Actually,
/usr/share/perl5/Test/Builder.pm has

sub details {
my $self = shift;
return @{ $self->{Test_Results} };
}

and line 1798 is the "return" statement in that. I still lack enough
Perl-fu to decipher this, though.

I think it's complaining about being called as a class method. Perhaps
this would work:

+ foreach my $detail (Test::More->builder->details())

Yes, that's the problem. Instead of using details(), summary() is
enough actually. And it is enough to let caller know the failure when
just one test has been found as not passing. See attached.

Thanks.

I'm setting up a container with Debian Wheezy, which looks old enough to
test proposed framework changes, so I'll be able to test future patches
against a real Perl 5.8.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Michael Paquier <michael.paquier@gmail.com> writes:

Yes, that's the problem. Instead of using details(), summary() is
enough actually. And it is enough to let caller know the failure when
just one test has been found as not passing. See attached.

This one works for me on RHEL6. Pushed; we'll see if the older
buildfarm members like it.

regards, tom lane

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