On Tuesday, March 8, 2022 10:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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:
Thank you for checking !
> ===============
> 1.
> + ereport(LOG,
> + errmsg("logical replication subscription \"%s\" has been be disabled
> due to an error",
>
> Typo.
> /been be/been
Fixed.
> 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..."
I thought it's ok without the change at the beginning, but I was wrong.
To make this new option aligned with others, I should add one check
for this feature. Fixed.
> 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.
Fixed and combined them together.
> 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.
Fixed.
The new patch is shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373824855A6C4D2178027A0ED0A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
