On Tue, Mar 8, 2022 at 1:37 PM osumi.takami...@fujitsu.com <osumi.takami...@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.