Hi, I just started looking this over a bit. The first thing I noticed
is that it adds a dependency on Archive::Tar which isn't already used
anywhere else. Did anybody check whether this exists back in 5.8
installations?
Why is "recovery" added to ALWAYS_SUBDIRS in src/test/Makefile instead
of to SUBDIRS? Seems a strange choice.
Instead of adding print "# Error output: $stderr\n" if $stderr ne "";
to sub psql, I think it would be better to add line separators, which
would be clearer if the error output ever turns into a multiline error
messages. It would still show as empty if no stderr is produced; so I
think something like
if ($stderr ne '')
{print "#### Begin standard error\n"print $stderr;print "#### End standard error\n";
}
or something like that.
In my days of Perl, it was starting to become frowned upon to call
subroutines without parenthesizing arguments. Is that no longer the
case? Because I notice there are many places in this patch and pre-
existing that call psql with an argument list without parens. And it's
a bit odd because I couldn't find any other subroutine that we're using
in that way.
In 005_replay_delay there's a 2s delay configured; then we test whether
something is replayed in 1s. I hate tests that run for a long time, but
is 2s good enough considering that some of our test animals in buildfarm
are really slow?
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
