Re: Optionally automatically disable logical replication subscriptions on error

On Thu, Nov 11, 2021 at 8:20 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> C codes are checked by pgindent.
>
> Note that this depends on the v20 skip xide patch in [1]
>

Some comments on the v4 patch:

(1) Patch subject
I think the patch subject should say "disable" instead of "disabling":
Optionally disable subscriptions on error

doc/src/sgml/ref/create_subscription.sgml
(2) spelling mistake
+ if replicating data from the publisher triggers non-transicent errors

non-transicent -> non-transient

(I notice Vignesh also pointed this out)

src/backend/replication/logical/worker.c
(3) calling geterrcode()
The new IsSubscriptionDisablingError() function calls geterrcode().
According to the function comment for geterrcode(), it is only
intended for use in error callbacks.
Instead of calling geterrcode(), couldn't the ErrorData from PG_CATCH
block be passed to IsSubscriptionDisablingError() instead (from which
it can get the sqlerrcode)?

(4) DisableSubscriptionOnError
DisableSubscriptionOnError() is again calling MemoryContextSwitch()
and CopyErrorData().
I think the ErrorData from the PG_CATCH block could simply be passed
to DisableSubscriptionOnError() instead of the memory-context, and the
existing MemoryContextSwitch() and CopyErrorData() calls could be
removed from it.

AFAICS, applying (3) and (4) above would make the code a lot cleaner.

Regards,
Greg Nancarrow
Fujitsu Australia