RE: Optionally automatically disable logical replication subscriptions on error

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