Re: 024_add_drop_pub.pl might fail due to deadlock

On Wed, Jul 23, 2025 at 2:42 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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.