On Tue, Mar 8, 2022 at 1:37 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> Kindly have a look at v30.
>
Review comments:
===============
1.
+ ereport(LOG,
+ errmsg("logical replication subscription \"%s\" has been be disabled
due to an error",
Typo.
/been be/been
2. Is there a reason the patch doesn't allow workers to restart via
maybe_reread_subscription() when this new option is changed, if so,
then let's add a comment for the same? We currently seem to be
restarting the worker on any change via Alter Subscription. If we
decide to change it for this option as well then I think we need to
accordingly update the current comment: "Exit if any parameter that
affects the remote connection was changed." to something like "Exit if
any parameter that affects the remote connection or a subscription
option was changed..."
3.
if (fout->remoteVersion >= 150000)
- appendPQExpBufferStr(query, " s.subtwophasestate\n");
+ appendPQExpBufferStr(query, " s.subtwophasestate,\n");
else
appendPQExpBuffer(query,
- " '%c' AS subtwophasestate\n",
+ " '%c' AS subtwophasestate,\n",
LOGICALREP_TWOPHASE_STATE_DISABLED);
+ if (fout->remoteVersion >= 150000)
+ appendPQExpBuffer(query, " s.subdisableonerr\n");
+ else
+ appendPQExpBuffer(query,
+ " false AS subdisableonerr\n");
It is better to combine these parameters. I see there is a similar
coding pattern for 14 but I think that is not required.
4.
+$node_subscriber->safe_psql('postgres', qq(ALTER SUBSCRIPTION sub ENABLE));
+
+# Wait for the data to replicate.
+$node_subscriber->poll_query_until(
+ 'postgres', qq(
+SELECT COUNT(1) = 1 FROM pg_catalog.pg_subscription_rel sr
+WHERE sr.srsubstate IN ('s', 'r') AND sr.srrelid = 'tbl'::regclass));
See other scripts like t/015_stream.pl and wait for data replication
in the same way. I think there are two things to change: (a) In the
above query, we use NOT IN at other places (b) use
$node_publisher->wait_for_catchup before this query.
--
With Regards,
Amit Kapila.
