On Thursday, January 6, 2022 12:17 PM Tang, Haiying/唐 海英 <tanghy.f...@fujitsu.com> wrote: > Thanks for updating the patch. Here are some comments: Thank you for your review !
> 1) > + /* > + * We would not be here unless this subscription's disableonerr field > was > + * true when our worker began applying changes, but check whether > that > + * field has changed in the interim. > + */ > + if (!subform->subdisableonerr) > + { > + /* > + * Disabling the subscription has been done already. No need > of > + * additional work. > + */ > + heap_freetuple(tup); > + table_close(rel, RowExclusiveLock); > + CommitTransactionCommand(); > + return; > + } > > I don't understand what does "Disabling the subscription has been done > already" > mean, I think we only run here when subdisableonerr is changed in the interim. > Should we modify this comment? Or remove it because there are already some > explanations before. Removed. The description you pointed out was redundant. > 2) > + /* Set the subscription to disabled, and note the reason. */ > + values[Anum_pg_subscription_subenabled - 1] = > BoolGetDatum(false); > + replaces[Anum_pg_subscription_subenabled - 1] = true; > > I didn't see the code corresponding to "note the reason". Should we modify the > comment? Fixed the comment by removing the part. We come here when an error occurred and the reason is printed as log so no need to note more reason. > 3) > + bool disableonerr; /* Whether errors automatically > disable */ > > This comment is hard to understand. Maybe it can be changed to: > > Indicates if the subscription should be automatically disabled when > subscription workers detect any errors. Agreed. Fixed. Kindly have a look at the attached v16. Best Regards, Takamichi Osumi
v16-0001-Optionally-disable-subscriptions-on-error.patch
Description: v16-0001-Optionally-disable-subscriptions-on-error.patch