RE: Optionally automatically disable logical replication subscriptions on error

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