Re: Optionally automatically disable logical replication subscriptions on error

On Tue, Feb 22, 2022 at 3:03 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> ~~~
>
> 1. worker.c - comment
>
> + subform = (Form_pg_subscription) GETSTRUCT(tup);
> +
> + /*
> + * We would not be here unless this subscription's disableonerr field was
> + * true, but check whether that field has changed in the interim.
> + */
> + if (!subform->subdisableonerr)
> + {
> + heap_freetuple(tup);
> + table_close(rel, RowExclusiveLock);
> + CommitTransactionCommand();
> + return false;
> + }
>
> I felt that comment belongs above the subform assignment because that
> is the only reason we are getting the subform again.

IIUC if we return false here, the same error will be emitted twice.
And I'm not sure this check is really necessary. It would work only
when the subdisableonerr is set to false concurrently, but doesn't
work for the opposite caces. I think we can check
MySubscription->disableonerr and then just update the tuple.

Here are some comments:

Why do we need SyncTableStartWrapper() and ApplyLoopWrapper()?

---
+ /*
+ * Log the error that caused DisableSubscriptionOnError to be called. We
+ * do this immediately so that it won't be lost if some other internal
+ * error occurs in the following code.
+ */
+ EmitErrorReport();
+ AbortOutOfAnyTransaction();
+ FlushErrorState();

Do we need to hold interrupts during cleanup here?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/