Обсуждение: refactor subscription tests to use PostgresNode's wait_for_catchup

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.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On 1/8/18 23:47, Michael Paquier wrote:
>> @@ -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?

Should we just remove it?  Apparently, it was never functional to begin
with.  Otherwise, we'd have to write a second query to return the value
to print.  wait_for_slot_catchup has the same issue.  Seems like a lot
of overhead for something that has never been used.

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

Right.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 10, 2018 at 09:45:56PM -0500, Peter Eisentraut wrote:
> On 1/8/18 23:47, Michael Paquier wrote:
> Should we just remove it?  Apparently, it was never functional to begin
> with.  Otherwise, we'd have to write a second query to return the value
> to print.  wait_for_slot_catchup has the same issue.  Seems like a lot
> of overhead for something that has never been used.

Fine for me to remove it.
--
Michael

On 1/10/18 22:24, Michael Paquier wrote:
> On Wed, Jan 10, 2018 at 09:45:56PM -0500, Peter Eisentraut wrote:
>> On 1/8/18 23:47, Michael Paquier wrote:
>> Should we just remove it?  Apparently, it was never functional to begin
>> with.  Otherwise, we'd have to write a second query to return the value
>> to print.  wait_for_slot_catchup has the same issue.  Seems like a lot
>> of overhead for something that has never been used.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services