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

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.

> - In 0002, added perldoc for new promote routine
> - In 0003, added perldoc documentation for the new options introduced
> in init and init_from_backup, and fixed some log entries not using the
> node name to identify the node involved when enabling archive,
> streaming or recovery.
>

Very much appreciated.

> - Craig has pinged me regarding tap_tests being incorrectly updated in
> config_default.pl in 0003.
> I just re-ran the tests on OSX and Windows (MSVC 2010 with Win7) to be
> sure that nothing broke, and nothing has been reported as broken.
>
>

I've looked over the tests. I see that you've updated the docs for the
Windows tests to reflect the changes, which is good, thanks.

I like the patch and would love to see it committed soon.

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.

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.

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.

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

Otherwise looks _really_ good and I'd love to see this committed very soon.

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

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