RE: Optionally automatically disable logical replication subscriptions on error

Thank you for checking the patch !

On Friday, November 12, 2021 1:49 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> On Thu, Nov 11, 2021 at 8:20 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> 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
Fixed.

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

> (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.
Fixed.

The updated patch is shared in [1].

[1] - https://www.postgresql.org/message-id/TYCPR01MB8373771371B31E1E6CC74B0AED999%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Takamichi Osumi