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.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
