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

On Thu, Mar 3, 2016 at 4:11 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> The first three are simple fixes that should go in without fuss:
>
> 001 fixes the above 5.8.8 compat issue.
> 002  fixes another minor whoopsie, a syntax error in
> src/test/recovery/t/003_recovery_targets.pl that never got noticed because
> exit codes are ignored.

Those two are embarrassing things... However:
-$node_master->psql('postgres',
-       "SELECT pg_create_restore_point('$recovery_name'");
+$node_master->psql_check('postgres',
+       "SELECT pg_create_restore_point('$recovery_name');");
In 0002 this is not correct, psql_check is part of a routine you
introduce later on.

> The rest are feature patches:
> 004 allows filtering on RecursiveCopy by a predicate function. Needed for
> filesystem level backups (007). It could probably be squashed with 007 if
> desired.

Adding perldoc to this module should be done separately, let's not mix
things. Applying a filter is useful as well to remove for example the
contents of pg_xlog, so no objections to it.

> 005 adds the new psql functions psql_expert and psql_check. Then 006 renames
> psql_expert to psql and fixes up all call sites across the tree to use the
> new interface. I found the bug in 002 as part of that process. I anticipate
> that 005 and 006 would be squashed into one commit to master, but I've kept
> them separate in my tree for easier review.

psql_check sounds wrong to me. I thought first that this triggers a
test. Why not psql_simple or psql_basic, or just keep psql.

> 007 adds PostgresNode support for hot and cold filesystem-level backups
> using pg_start_backup and pg_stop_backup, which will be required for some
> coming tests and are useful by themselves.

It would be nice as well to have a flag in _backup_fs to filter the
contents of pg_xlog at will.

log_collector is not enabled in the nodes
deployed by PostgresNode, so why filtering it?