Thank you for checking the patch ! On Friday, November 12, 2021 1:49 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > On Thu, Nov 11, 2021 at 8:20 PM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.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
