Re: refactor subscription tests to use PostgresNode's wait_for_catchup
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
Re: refactor subscription tests to use PostgresNode's wait_for_catchup
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 signature.asc Description: PGP signature
Re: refactor subscription tests to use PostgresNode's wait_for_catchup
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
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
signature.asc
Description: PGP signature
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
From daf3c2d02a409a33bff349c558d79dcdd966982e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Mon, 8 Jan 2018 17:32:09 -0500
Subject: [PATCH] Refactor subscription tests to use PostgresNode's
wait_for_catchup
---
src/test/perl/PostgresNode.pm | 16 +---
src/test/subscription/t/001_rep_changes.pl | 19 +--
src/test/subscription/t/002_types.pl | 15 ---
src/test/subscription/t/003_constraints.pl | 15 ---
src/test/subscription/t/004_sync.pl| 14 +++---
src/test/subscription/t/005_encoding.pl| 13 ++---
src/test/subscription/t/006_rewrite.pl | 17 -
src/test/subscription/t/007_ddl.pl | 11 +--
src/test/subscription/t/008_diff_schema.pl | 15 +++
9 files changed, 39 insertions(+), 96 deletions(-)
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 80f68df246..6062f41480 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1465,7 +1465,8 @@ sub lsn
=item $node->wait_for_catchup(standby_name, mode, target_lsn)
-Wait for the node with application_name standby_name (usually from node->name)
+Wait for the node with application_name standby_name (usually from node->name,
+also works for logical subscriptions)
until its replication location in pg_stat_replication equals or passes the
upstream's WAL insert point at the time this function is called. By default
the replay_lsn is waited for, but 'mode' may be specified to wait for any of
@@ -1477,6 +1478,7 @@ poll_query_until timeout.
Requires that the 'postgres' db exists and is accessible.
target_lsn may be any arbitrary lsn, but is typically
$master_node->lsn('insert').
+If omitted, pg_current_wal_lsn() is used.
This is not a test. It die()s on failure.
@@ -1497,7 +1499,15 @@ sub wait_for_catchup
{
$standby_name = $standby_name->name;
}
- die 'target_lsn must be specified' unless defined($target_lsn);
+ my $lsn_expr;
+ if (defined($target_lsn))
+ {
+ $lsn_expr = "'$target_lsn'";
+ }
+ else
+ {
+ $lsn_expr = 'pg_current_wal_lsn()'
+ }
print "Waiting for replication conn "
. $standby_name . "'s "
. $mode
@@ -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)');
diff --git a/src/test/subscription/t/001_rep_changes.pl
b/src/test/subscription/t/001_rep_changes.pl
index 0136c79d4b..e0104cd8d0 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -60,11 +60,7 @@
"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
application_name=$appname' PUBLICATION tap_pub, tap_pub_ins_only"
);
-# Wait for subscriber to finish initialization
-my $caughtup_query =
-"SELECT pg_current_wal_lsn() <= replay_lsn FROM pg_stat_replication WHERE
application_name = '$appname';";
-$node_publisher->poll_query_until('postgres', $caughtup_query)
- or die "Timed out while waiting for subscriber to catch up";
+$node_publisher->wait_for_catchup($appname);
# Also wait for initial table sync to finish
my $synced_query =
@@ -93,8 +89,7 @@
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_mixed VALUES (2, 'bar')");
-$node_publisher->poll_query_until('postgres', $caughtup_query)
- or die "Timed out while waiting for subscriber to catch up";
+$node_publisher->wait_for_catchup($appname);
$result = $node_subscriber->safe_psql('postgres',
"SELECT count(*), min(a), max(a) FROM tab_ins");
@@ -132,9 +127,7 @@
$node_publisher->safe_psql('postgres',
"UPDATE tab_full2 SET x = 'bb' WHERE x = 'b'");
-# Wait for subscription to catch up
-$node_publisher->poll_query_until('postgres', $caughtup_query)
- or die "Time
