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

On 3 March 2016 at 21:16, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> On Thu, Mar 3, 2016 at 4:11 PM, Craig Ringer <craig(at)2ndquadrant(dot)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.
>

Damn, I meant to fix that when I rebased it back in the history but forgot.
Thankyou.

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

Eh, ok. I figured it was so trivial it didn't matter, but will split.

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

I guess I'm used to Python's subprocess.check_call so to me it's natural.

I want something that makes it clear that failure is a fatal error
condition, i.e. "do this in psql and if it produces an error, treat it like
you would any other error in Perl and die appropriately".

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

i.e. copy without any pg_xlog at all? without_xlog => 1 ?

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

Because at the time I wrote that the test dirs were deleted unconditionally
and I didn't bother checking if we actually wrote anything to pg_log. Also,
because if it _does_ get enabled by someone I can't imagine why we'd want
to copy the logs.

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