RE: Optionally automatically disable logical replication subscriptions on error

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