On Tue, 22 Apr 2025 at 16:31, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> > 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');
>
> I added it to ensure remote_lsn to the valid value, but yes it is not mandatory.
>
> > 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.
>
> I intentionally used poll_query_until(), but I have no strong opinions.
> Modified.
>
> PSA new patches.
Thanks for the updated patch. I’ve reviewed and verified the changes
across the HEAD, PG17, and PG16 branches, and everything is working as
expected. I have no further comments.
Regards,
Vignesh
