RE: Optionally automatically disable logical replication subscriptions on error

On Wednesday, January 5, 2022 8:53 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威
> <wangw(dot)fnst(at)fujitsu(dot)com> wrote:
> > On Thursday, December 16, 2021 8:51 PM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)com> 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.
>

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