Re: refactor subscription tests to use PostgresNode's wait_for_catchup

On Mon, Jan 08, 2018 at 09:46:21PM -0500, Peter Eisentraut wrote:
> It appears that we have unwittingly created some duplicate and
> copy-and-paste-prone code in src/test/subscription/ to wait for a
> replication subscriber to catch up, when we already have
> almost-sufficient code in PostgresNode to do that more compactly. So I
> propose this patch to consolidate that.

This looks sane to me. I have two comments while I read the
surroundings.

> @@ -1505,7 +1515,7 @@ sub wait_for_catchup
> . $target_lsn . " on "
> . $self->name . "\n";
> my $query =
> -qq[SELECT '$target_lsn' <= ${mode}_lsn FROM pg_catalog.pg_stat_replication WHERE application_name = '$standby_name';];
> +qq[SELECT $lsn_expr <= ${mode}_lsn FROM pg_catalog.pg_stat_replication WHERE application_name = '$standby_name';];
> $self->poll_query_until('postgres', $query)
> or die "timed out waiting for catchup, current location is "
> . ($self->safe_psql('postgres', $query) || '(unknown)');

This log is wrong from the beginning. Here $query returns a boolean
status and not a location. I think that when the poll dies because of a
timeout you should do a lookup at ${mode}_lsn from pg_stat_replication
when application_name matching $standby_name. Could you fix that as
well?

Could you also update promote_standby in RewindTest.pm? Your refactoring
to use pg_current_wal_lsn() if a target_lsn is not possible makes this
move possible. Using the generic APIs gives better logs as well.
--
Michael