Re: [REVIEW] In-core regression tests for replication, cascading, archiving, PITR, etc.

On Fri, Feb 26, 2016 at 1:47 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 26 February 2016 at 10:58, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>> Here is a rebased set after the conflicts created by e640093, with the
>> following changes:
>
> Thanks for rebasing on top of that. Not totally fair when your patch came
> first, but I guess it was simpler to merge the other one first.

At this point the final result is the same. It does not matter what
gets in first.

> I do have one major disagreement, which is that you turn autovacuum off if
> streaming is enabled. This is IMO completely wrong and must be removed. It's
> making the tests ignore a major and important part of real-world use.

This has been chosen for consistency with what is in pg_rewind tests,
the idea being to keep the runs more stable with a WAL output under
control to allow predictable results. Though I do not see any direct
reason to not remove it actually now that I think about it.

> If you did it to make it easier to test replay catchup etc, just use
> pg_xlog_location_diff instead of an equality test. Instead of:
> my $caughtup_query = "SELECT '$current_lsn'::pg_lsn <=
> pg_last_xlog_replay_location()";
> use
> my $caughtup_query = "SELECT pg_xlog_location_diff('$current_lsn',
> pg_last_xlog_replay_location()) <= 0";
> so it doesn't care if we replay past the expected LSN on the master due to
> autovacuum activity. That's what's done in the real world and what should be
> covered by the tests IMO.

Those two statements have the same meaning. pg_xlog_location_diff does
exactly the same thing as the pg_lsn data type in terms of LSN
comparisons. Choosing one or the other is really a matter of taste.
Though I see that 001 is the only test that uses an equality, this
should not be the case I agree.

> The patch sets
>
> tap_tests => 1,
>
> in config_default.pl. Was that on purpose? I'd have no problem with running
> the TAP tests by default if they worked by default, but the docs say that at
> least with ActiveState's Perl you have to jump through some hoops to get
> IPC::Run.

No, this was an error in the previous version of the patch 0003. Those
tests should be disabled by default, to match what ./configure does,
and also because installing IPC::Run requires some extra operations,
but that's easily doable with a bit of black magic.

> Typo in PostgresNode.pm: passiong should be 'passing'.

Oops.

> I'd like a way to append parameters in a way that won't clobber settings
> made implicitly by the module through things like enable_streaming but I can
> add that in a followup patch. It doesn't need to complicate this one.

This is something that I have been thinking about for some time while
hacking this thing, but I finished with the current version to not
complicate the patch more than it needs to be, and because the current
version is enough for the needs of all the tests present. Surely this
can be extended further more. One idea that I had was for example to
pass as parameter to init() and init_from_backup() a set of key/values
that would be appended to postgresql.conf.

> I'm thinking of having the tests append an include_dir directive when they
> create a node, maintain a hash of all parameters and rewrite a
> postgresql.conf.taptests file in the include_dir when params are updated.
> Also exposing a 'reload' call.

The reload wrapper would make sense to have. That has not proved to be
necessary yet.
--
Michael