Subject: RE: Optionally automatically disable logical replication subscriptions on error

From: "[email-masked]" <[email-masked]>
Date: Wed, 5 Jan 2022 12:53:06 +0000

On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威 <[email-masked]> wrote:
> On Thursday, December 16, 2021 8:51 PM [email-masked]
> <[email-masked]> wrote:
> > Attached the updated patch v14.
> 
> A comment to the timing of printing a log:
Thank you for your review !

> After the log[1] was printed, I altered subscription's option
> (DISABLE_ON_ERROR) from true to false before invoking
> DisableSubscriptionOnError to disable subscription. Subscription was not
> disabled.
> [1] "LOG:  logical replication subscription "sub1" will be disabled due to an
> error"
> 
> I found this log is printed in function WorkerErrorRecovery:
> +	ereport(LOG,
> +			errmsg("logical replication subscription \"%s\" will
> be disabled due to an error",
> +				   MySubscription->name));
> This log is printed here, but in DisableSubscriptionOnError, there is a check to
> confirm subscription's disableonerr field. If disableonerr is found changed from
> true to false in DisableSubscriptionOnError, subscription will not be disabled.
> 
> In this case, "disable subscription" is printed, but subscription will not be
> disabled actually.
> I think it is a little confused to user, so what about moving this message after
> the check which is mentioned above in DisableSubscriptionOnError?
Makes sense. I moved the log print after
the check of the necessity to disable the subscription.

Also, I've scrutinized and refined the new TAP test as well for refactoring.
As a result, I fixed wait_for_subscriptions()
so that some extra codes that can be simplified,
such as escaped variable and one part of WHERE clause, are removed.
Other change I did is to replace two calls of wait_for_subscriptions()
with polling_query_until() for the subscriber, in order to
make the tests better and more suitable for the test purposes.
Again, for this refinement, I've conducted a tight loop test
to check no timing issue and found no problem.

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 6 Jan 2022 03:16:30 +0000

> to check no timing issue and found no problem.

Thanks for updating the patch. Here are some comments:

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.

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?

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.

Regards,
Tang

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 6 Jan 2022 05:53:42 +0000

> Should we modify this comment? Or remove it because there are already some
Removed. The description you pointed out was redundant.

> I didn't see the code corresponding to "note the reason". Should we modify the
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.

> 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