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