Re: Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

On Thu, Nov 19, 2015 at 12:21 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
>
> 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?

Actually I didn't and that's a good point, we have decided to support
TAP down to 5.8.9. The only reason why I introduced this dependency is
that there is no easy native way to copy an entire folder in perl, and
that's for handling base backups. There are things like File::NCopy of
File::Copy::Recursive however it does not seem like a good idea to
depend on other modules that IPC::Run. Would it be better to have an
in-core module dedicated to that similar to SimpleTee.pm? Or are you
guys fine to accept a dependency with another module?

> Why is "recovery" added to ALWAYS_SUBDIRS in src/test/Makefile instead
> of to SUBDIRS? Seems a strange choice.

Because I thought that it should not be part of the main regression
suite, like ssl/. Feel free to correct me if my feeling is wrong.

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

Yes, that would be better.

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

Hm, yeah. If we decide about a perl coding policy I would be happy to
follow it. Personally I prefer usually using parenthesis however if we
decide to make the calls consistent we had better address that as a
separate patch.

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

A call to poll_query_until ensures that we wait for the standby to
replay once the minimum replay threshold is reached. Even with a slow
machine the first query would still see only 10 rows at the first try,
and then wait for the standby to replay before checking if 20 rows are
visible. Or I am not following your point.
--
Michael