On Thursday, January 6, 2022 12:17 PM Tang, Haiying/唐 海英 <tanghy.fnst@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
