On Tue, Mar 8, 2022 at 9:37 AM Peter Smith <smithpb2250@gmail.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.
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.
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.
--
With Regards,
Amit Kapila.