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.
