RE: Optionally automatically disable logical replication subscriptions on error

On Tuesday, March 8, 2022 1:07 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Please find below some review comments for v29.
Thank you for your comments !

> ======
>
> 1. src/backend/replication/logical/worker.c - worker_post_error_processing
>
> +/*
> + * Abort and cleanup the current transaction, then do post-error processing.
> + * This function must be called in a PG_CATCH() block.
> + */
> +static void
> +worker_post_error_processing(void)
>
> The function comment and function name are too vague/generic. I guess this is
> a hang-over from Sawada-san's proposed patch, but now since this is only
> called when disabling the subscription so both the comment and the function
> name should say that's what it is doing...
>
> e.g. rename to DisableSubscriptionOnError() or something similar.
Fixed the comments and the function name in v30 shared in [1].

> ~~~
>
> 2. src/backend/replication/logical/worker.c - worker_post_error_processing
>
> + /* Notify the subscription has been disabled */ ereport(LOG,
> + errmsg("logical replication subscription \"%s\" has been be disabled
> due to an error",
> + MySubscription->name));
>
> proc_exit(0);
> }
>
> I know this is common code, but IMO it would be better to do the proc_exit(0);
> from the caller in the PG_CATCH. Then I think the code will be much easier to
> read the throw/exit logic, rather than now where it is just calling some function
> that never returns...
>
> Alternatively, if you want the code how it is, then the function name should give
> some hint that it is never going to return - e.g.
> DisableSubscriptionOnErrorAndExit)
I renamed it to DisableSubscriptionAndExit in the end
according to the discussion.

> ~~~
>
> 3. src/backend/replication/logical/worker.c - start_table_sync
>
> + {
> + /*
> + * Abort the current transaction so that we send the stats message
> + * in an idle state.
> + */
> + AbortOutOfAnyTransaction();
> +
> + /* Report the worker failed during table synchronization */
> + pgstat_report_subscription_error(MySubscription->oid, false);
> +
> + PG_RE_THROW();
> + }
>
> (This is a repeat of a previous comment from [1] comment #2)
>
> I felt the separation of those 2 statements and comments makes the code less
> clean than it could/should be. IMO they should be grouped together.
>
> SUGGESTED
>
> /*
> * Report the worker failed during table synchronization. Abort the
> * current transaction so that the stats message is sent in an idle
> * state.
> */
> AbortOutOfAnyTransaction();
> pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_work
> er());
Fixed.

> ~~~
>
> 4. src/backend/replication/logical/worker.c - start_apply
>
> + {
> + /*
> + * Abort the current transaction so that we send the stats message
> + * in an idle state.
> + */
> + AbortOutOfAnyTransaction();
> +
> + /* Report the worker failed while applying changes */
> + pgstat_report_subscription_error(MySubscription->oid,
> + !am_tablesync_worker());
> +
> + PG_RE_THROW();
> + }
>
> (same as #3 but comment says "while applying changes")
>
> SUGGESTED
>
> /*
> * Report the worker failed while applying changing. Abort the current
> * transaction so that the stats message is sent in an idle state.
> */
> AbortOutOfAnyTransaction();
> pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_work
> er());
Fixed. I choose the woring "while applying changes" which you mentioned first
and sounds more natural.

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

Best Regards,
Takamichi Osumi