RE: Optionally automatically disable logical replication subscriptions on error

On Tuesday, March 8, 2022 2:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Mar 8, 2022 at 9:37 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > Please find below some review comments for v29.
> >
> > ======
> >
> > 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.
> >
> > ~~~
> >
> > 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 think we are already in error so maybe it is better to name it as
> DisableSubscriptionAndExit.
OK. Renamed.

> Few other comments:
> =================
> 1.
> DisableSubscription()
> {
> ..
> +
> + LockSharedObject(SubscriptionRelationId, subid, 0,
> + AccessExclusiveLock);
>
> Why do we need AccessExclusiveLock here? The Alter/Drop Subscription
> takes AccessExclusiveLock, so AccessShareLock should be sufficient unless
> we have a reason to use AccessExclusiveLock lock. The other similar usages in
> this file (pg_subscription.c) also take AccessShareLock.
Fixed.

> 2. Shall we mention this feature in conflict handling docs [1]:
> Now:
> To skip the transaction, the subscription needs to be disabled temporarily by
> ALTER SUBSCRIPTION ... DISABLE first.
>
> After:
> To skip the transaction, the subscription needs to be disabled temporarily by
> ALTER SUBSCRIPTION ... DISABLE first or alternatively, the subscription can
> be used with the disable_on_error option.
>
> Feel free to use something on the above lines, if you agree.
Agreed. Fixed.

At the same time, the attached v30 has incorporated
some rebase results of recent commit(d3e8368)
so that start_table_sync allocates the origin names
in long-lived context. Accoring to this, I modified
some comments on this function.

I made some comments for sending stats in
start_table_sync and start_apply united and concise,
which were pointed out by Peter Smith in [1].

[1] - https://www.postgresql.org/message-id/CAHut%2BPs3b8HjsVyo-Aygtnxbw1PiVOC9nvrN6dKxYtS4C8s%2Bgw%40mail.gmail.com

Kindly have a look at v30.

Best Regards,
Takamichi Osumi