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

On 2 March 2016 at 10:07, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 2 March 2016 at 05:46, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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