On 3 March 2016 at 21:16, Michael Paquier <michael.paqu...@gmail.com> wrote:
> On Thu, Mar 3, 2016 at 4:11 PM, Craig Ringer <cr...@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. > 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