Re: 024_add_drop_pub.pl might fail due to deadlock

On Wed, Jul 23, 2025 at 2:42 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Ajin,
>
> Thanks for the patch. Firstly let me confirm my understanding. While altering the
> subscription, locks are acquired with below ordering:
>
> Order target level
> 1 pg_subscription row exclusive
> 2 pg_subscription, my tuple access exclusive
> 3 pg_subscription_rel access exclusive
> 4 pg_replication_origin excluive
>
> In contrast, apply worker works like:
>
> Order target level
> 1 pg_replication_origin excluive
> 2 pg_subscription, my tuple access share
> 3 pg_subscrition_rel row exclusive
>
> Thus a backend may wait at step 4, and apply worker can stuck at step 2 or 3.
>
> Below are my comments:
>
> ```
> @@ -340,7 +341,6 @@ process_syncing_tables_for_sync(XLogRecPtr current_lsn)
> * is dropped. So passing missing_ok = false.
> */
> ReplicationSlotDropAtPubNode(LogRepWorkerWalRcvConn, syncslotname, false);
> -
> ```
> This change is not needed.
>
> ```
> + if (!rel)
> + rel = table_open(SubscriptionRelRelationId, AccessExclusiveLock);
> +
> ```
>
> I feel it is too strong, isn't it enough to use row exclusive as initially used?
>
> ```
> UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
> - XLogRecPtr sublsn)
> + XLogRecPtr sublsn, bool lock_needed)
> ```
>
> I feel the name `lock_needed` is bit misleading, because the function still opens
> the pg_subscription_rel catalog with row exclusive. How about "lock_shared_object"?
>

I think if we lock in a caller, we don't need to use any lock during
table_open. We can use the parameter name as already_locked as we do
at some other places in the code.

--
With Regards,
Amit Kapila.