On Tue, 22 Apr 2025 at 14:36, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Sawada-san,
>
> > Thank you for the patch. The changes look reasonable to me. Can we add
> > some regression tests for that?
> >
>
> Yeah it should be added. I think it is enough to add the first scenario. Done in
> 100_bugs.pl.
> I also ran pgindent/pgperltidy. Because of self-reviewing the version became v4.
Couple of minor suggestions in the test:
1) I felt this is not required for this test as it has been verified many times:
+
+# Insert tuples and confirms replication works well
+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
+
+$node_publisher->wait_for_catchup('regress_sub');
2) How about we change:
+# Confirms the origin can be advanced
+ok( $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT remote_lsn > '$remote_lsn' FROM
pg_catalog.pg_replication_origin_status os, pg_catalog.pg_subscription
s WHERE os.external_id = 'pg_' || s.oid AND s.subname =
'regress_sub'",
+ 't')
+ or die "Timed out while waiting for replication origin to be
updated");
to:
$node_publisher->wait_for_catchup('regress_sub');
# Confirms the origin can be advanced
$result = $node_subscriber->safe_psql('postgres',
"SELECT remote_lsn > '$remote_lsn' FROM
pg_catalog.pg_replication_origin_status os, pg_catalog.pg_subscription
s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub'"
);
is($result, 't',
'remote_lsn has advanced for apply worker raising an exception');
In that way, we need not wait on poll_query_until.
Regards,
Vignesh
