On Thursday, December 16, 2021 2:32 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > On Tue, Dec 14, 2021 at 4:34 PM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.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
v14-0001-Optionally-disable-subscriptions-on-error.patch
Description: v14-0001-Optionally-disable-subscriptions-on-error.patch