Re: Optionally automatically disable logical replication subscriptions on error

On Thu, Jan 6, 2022 at 11:23 AM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> Kindly have a look at the attached v16.
>

Few comments:
=============
1.
@@ -3594,13 +3698,29 @@ ApplyWorkerMain(Datum main_arg)
    apply_error_callback_arg.command,
    apply_error_callback_arg.remote_xid,
    errdata->message);
- MemoryContextSwitchTo(ecxt);
+
+ if (!MySubscription->disableonerr)
+ {
+ /*
+ * Some work in error recovery work is done. Switch to the old
+ * memory context and rethrow.
+ */
+ MemoryContextSwitchTo(ecxt);
+ PG_RE_THROW();
+ }
  }
+ else if (!MySubscription->disableonerr)
+ PG_RE_THROW();

- PG_RE_THROW();

Can't we combine these two different checks for
'MySubscription->disableonerr' if you do it as a separate if check
after sending the stats message?

2. Can we move the code related to tablesync worker and its error
handing (the code insider if (am_tablesync_worker())) to a separate
function say LogicalRepHandleTableSync() or something like that.

3. Similarly, we can move apply-loop related code ("Run the main
loop.") to a separate function say LogicalRepHandleApplyMessages().

If we do (2) and (3), I think the code in ApplyWorkerMain will look
better. What do you think?

-- 
With Regards,
Amit Kapila.