RE: Optionally automatically disable logical replication subscriptions on error

On Thursday, December 16, 2021 2:32 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> On Tue, Dec 14, 2021 at 4:34 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> >
> > Besides all of those changes, I've removed the obsolete comment of
> > DisableSubscriptionOnError in v12.
> >
>
> I have a few minor comments, otherwise the patch LGTM at this point:
Thank you for your review !

> doc/src/sgml/catalogs.sgml
> (1)
> Current comment says:
>
> + If true, the subscription will be disabled when subscription's
> + worker detects any errors
>
> However, in create_subscription.sgml, it says "disabled if any errors are
> detected by subscription workers ..."
>
> For consistency, I think it should be:
>
> + If true, the subscription will be disabled when subscription
> + workers detect any errors
Okay. Fixed.

> src/bin/psql/describe.c
> (2)
> I think that:
>
> + gettext_noop("Disable On Error"));
>
> should be:
>
> + gettext_noop("Disable on error"));
>
> for consistency with the uppercase/lowercase usage on other similar entries?
> (e.g. "Two phase commit")
Agreed. Fixed.

> src/include/catalog/pg_subscription.h
> (3)
>
> + bool subdisableonerr; /* True if apply errors should disable the
> + * subscription upon error */
>
> The comment should just say "True if occurrence of apply errors should disable
> the subscription"
Fixed.

Attached the updated patch v14.

Best Regards,
Takamichi Osumi