Re: Per-table resync for logical replication subscriptions

Hello Hayato,

Thank you for the review, v5 is attached.

> I found that the deadlock could happen between the tablesync worker and
the
> REFRESH TABLE command.

Confirmed. Reproduced with a 3M row table so the copy is slow enough to
overlap: sub2 syncs and is disabled, sub1 starts copying the same table,
then
ALTER SUBSCRIPTION sub2 REFRESH TABLE t runs. On v4:

process 35095 still waiting for AccessExclusiveLock on relation 16384
of database 5 after 1000.458 ms
process 35072 detected deadlock while waiting for RowExclusiveLock on
relation 6102 of database 5 after 1001.136 ms
process 35095 acquired AccessExclusiveLock on relation 16384 of
database 5 after 4311.560 ms
process 35072 ERROR: deadlock detected

16384 is the table, 6102 is pg_subscription_rel. Worth spelling out who
died:
35072 was sub1's tablesync worker. The command killed a worker of a
subscription it was not operating on, and one it was going to refuse to
touch
anyway.

One reason this is easy to miss: the command's own output is identical on
both
builds. It reports the shared-relation rejection either way, and on v4 it
then
goes on to acquire the lock and finish. Only the server log shows that a
worker was terminated, which is why the script greps for it rather than
trusting the psql output.

> 4. The tablesync worker also tried to acquire AccessShare lock for
> pg_subscription_rel, but it would be blocked by the backend. It's done in
> copy_table()->logicalrep_rel_open()->GetSubscriptionRelState().

One small refinement, which only broadens your point. In my run the worker
was blocked on RowExclusiveLock rather than AccessShareLock, so it had got
past the read in GetSubscriptionRelState() and was updating its own state
via
UpdateSubscriptionRelState(). Both levels conflict with AccessExclusiveLock,
so the cycle is the same one; it just means any catalog access by the worker
closes it, read or write, and not only the call path you traced.

> One idea for the fix is to acquire AccessShare locks for user-defined
tables
> first, then acquire the AccessExclusive lock after the
> CheckRefreshTableNotShared(). This avoids to acquire strong locks only if
> it's needed, and my reproducer can reject by the function. Thought?

Agreed, and adopted. Relations are resolved with AccessShareLock, the check
runs, and only then:

foreach_oid(relid, relids)
LockRelationOid(relid, AccessExclusiveLock);

Your reproducer then rejects immediately with the shared-relation error,
sub1
keeps copying, and the log has no deadlock lines. It is also the better
shape
for the reason you give: the common rejection no longer waits for somebody
else's copy to finish before failing.

Three things I ran into while testing it, of which the second is the reason
v5 goes further than your suggestion.

First, the escalation makes this a lock upgrade, so it is worth showing that
two commands cannot sit on AccessShareLock and both ask for
AccessExclusiveLock. They cannot, for a structural reason rather than by
luck. AlterSubscription() takes

/* Lock the subscription so nobody else can do anything with it. */
LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);

before the switch on stmt->kind, so two REFRESH TABLE commands on the same
subscription are serialized before either touches a relation. Two commands
on
different subscriptions can only collide over a relation both subscriptions
feed, which is exactly what the check rejects before the escalation.
Measured
with two concurrent commands and the relation pinned by a third session:

locks on t:
72213 | AccessShareLock | t
72213 | AccessExclusiveLock | f
lock on the subscription object (classid 6100):
72213 | AccessExclusiveLock | t
72214 | AccessExclusiveLock | f

The second command never acquires AccessShareLock on t at all; it waits on
the
object lock.

Second, and this is the part that matters: your fix does not remove the
deadlock class on its own. The command still held AccessExclusiveLock on
pg_subscription_rel while waiting for the relation, and that alone is
enough.
Any backend holding a lock on one of these relations that then reads
pg_subscription_rel closes the same cycle, and it need not be a replication
worker:

BEGIN;
INSERT INTO t VALUES (-1); -- RowExclusiveLock on t
<pause>
SELECT count(*) FROM pg_subscription_rel;

run against REFRESH TABLE on a relation that passes the check, so the
escalation is actually reached:

Process 38605 waits for AccessShareLock on relation 6102 of database 5;
blocked by process 38722.
Process 38722 waits for AccessExclusiveLock on relation 16384 of
database 5; blocked by process 38605.
ERROR: deadlock detected

This time the user's transaction was the victim. No ordering of the two
locks
fixes this: whichever we take first, a transaction taking them in the other
order deadlocks. So v5 also weakens the catalog lock to the level this
function's updates actually need:

rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);

I had used AccessExclusiveLock to match AlterSubscription_refresh(), but the
two are not comparable: that function never waits for a lock on a user
relation while holding the catalog, and this one does. RowExclusiveLock does
not conflict with the AccessShareLock or RowExclusiveLock that workers and
ordinary backends take there, so the cycle cannot form. The same test now
commits normally and the command is accepted, with only an ordinary lock
wait
logged.

Third, that weakening is not free, and it is the one judgement call here I
would like your opinion on. RowExclusiveLock is also what CREATE
SUBSCRIPTION
takes, so it is no longer serialized against us, and CREATE SUBSCRIPTION ...
WITH (copy_data = false) records relations as SUBREL_STATE_READY. A relation
registered that way is never copied, so a registration landing after the
check
would leave the truncate discarding rows that nothing brings back.

What prevents it is that CREATE SUBSCRIPTION locks the relation before
registering it:

relid = RangeVarGetRelid(rv, AccessShareLock, false);
...
AddSubscriptionRelState(subid, relid, relation_state, ...);

so once this command has requested AccessExclusiveLock, no registration for
that relation can complete. Verified rather than assumed:

query
| mode | granted

-----------------------------------------------------------------+---------------------+--------
ALTER SUBSCRIPTION s1 REFRESH TABLE t | AccessShareLock | t
SELECT pg_sleep(8); |
AccessShareLock | t
ALTER SUBSCRIPTION s1 REFRESH TABLE t | AccessExclusiveLock | f
CREATE SUBSCRIPTION s2 CONNECTION ... | AccessShareLock | f

That leaves only the instant between the check's scan and the lock request,
so
v5 runs the check a second time once the relations are locked. The first
pass
is then purely the optimisation you asked for, and the second is the
conclusive one. REFRESH PUBLICATION still takes AccessExclusiveLock and so
remains serialized against us either way.

If you would rather keep AccessExclusiveLock on the catalog and accept the
user-transaction deadlock, it is a one line change and I will make it. I
preferred fixing the deadlock, since it can hit a transaction that has
nothing
to do with logical replication.

> 01. It might be matter of taste, but I feel the variable lock_mode is not
> needed: the pattern is mainly used when the mode can be different based on
> situations.

Removed. There are genuinely two levels now, and naming them at the sites
that
take them reads better than one variable whose meaning changes halfway
through.

> 02. The name CheckRefreshTableNotShared() is not suitable, because not
sure
> the meaning "Shared". How about CheckRefreshTableNotInOtherSubscriptions?

Renamed as you suggest.

> 03. get_partition_ancestors() seems to assume that the given relation has
at
> least one parent, but the patch does not ensure. Maybe
> get_rel_relispartition() or similar functions can be used.

You were right that this call needs a relispartition test, and
get_rel_relispartition() is what v5 now uses. The reason turned out to be a
different one than either of us had in mind, and it was a live bug, so let
me
lay out both halves.

It does not error on a relation with no parent. get_partition_ancestors()
calls get_partition_ancestors_worker(), which treats that as its base case,
because the top of any partition tree has no parent either:

/*
* Recursion ends at the topmost level, ie., when there's no parent; also
* when the partition is being detached.
*/
parentOid = get_partition_parent_worker(inhRel, relid, &detach_pending);
if (parentOid == InvalidOid || detach_pending)
return;

and get_partition_parent_worker() initialises its result to InvalidOid,
overwriting it only if the scan finds a tuple. The elog(ERROR) calls are in
get_partition_parent(), a different wrapper over the same worker, which this
path never calls.

The actual problem is what it returns when there is a parent but not a
partitioning one. get_partition_parent_worker() scans pg_inherits on
(inhrelid, inhseqno = 1) and does not look at relispartition, and
pg_inherits
records plain inheritance too. So for an INHERITS child it returns the
inheritance parent, and my code was treating that as a feeder. An
inheritance
parent does not route rows into its children, so that is wrong, and it is
observable: with tab_inh_c inheriting from tab_inh_p, and a second
subscription tracking ONLY tab_inh_p,

ALTER SUBSCRIPTION tap_sub_inh REFRESH TABLE tab_inh_c;
ERROR: table "tab_inh_c" is a partition of "tab_inh_p", which is part of
the subscription "tap_sub_inh_ponly"

A valid refresh is refused, and the message calls an inheritance child a
partition. It errs on the safe side, so no data was at risk, but there is no
way for the user to work around it.

v5 restricts the ancestor walk to real partitions:

if (get_rel_relispartition(relid))
feeders = lappend_oid(get_partition_ancestors(relid), relid);
else
feeders = list_make1_oid(relid);

The case above is now accepted, the genuine partition-ancestor rejection
still
fires, and 039 has a test for it that fails without the guard. Thank you for
pushing on this one; I had convinced myself it was only a style question.

> 04. You missed to update meson.build file.

Done.

> 05. Can you clarify the reason why the setting is required? Can we remove
if
> not needed?

Removed. I had copied it from 004_sync.pl.

Best regards,
Cagri

On Mon, Aug 17, 2026 at 11:09 AM Hayato Kuroda (Fujitsu) <
kuroda(dot)hayato(at)fujitsu(dot)com> wrote:

> Dear Cagri,
>
> Thanks for the update. I found this patch could cause an issue if two
> subscriptions
> are modifying the same table. The scenario:
>
> 1. A tablesync worker for sub1 acquired opened a table with the
> RowExclusive in LogicalRepSyncTableStart().
> 2. User ran ALTER SUBSCRIPTION sub2 REFRESH TABLE command.
> 3. The backend acquired a AccessExclusive lock for pg_subscription_rel,
> then
> tried to acquire a AccessExclusive Lock for tables. It would wait till
> the tablesync
> worker released.
> 4. The tablesync worker also tried to acquire AccessShare lock for
> pg_subscription_rel,
> but it would be blocked by the backend. It's done in
> copy_table()->logicalrep_rel_open()->GetSubscriptionRelState().
> 5. The deadlock detector detected the wait-for graph is now circle,
> thus it terminates either of them.
>
> One idea for the fix is to acquire AccessShare locks for user-defined
> tables
> first, then acquire the AccessExclusive lock after the
> CheckRefreshTableNotShared().
> This avoids to acquire strong locks only if it's needed, and my reproducer
> can
> reject by the function. Thought?
>
> Also, below are my cosmetic comments.
>
> 01.
> ```
> + LOCKMODE lockmode = AccessExclusiveLock;
> ```
>
> It might be matter of taste, but I feel the variable lock_mode is not
> needed:
> the pattern is mainly used when the mode can be different based on
> situations.
>
> 02.
> ```
> +static void
> +CheckRefreshTableNotShared(Relation pgsubrel, Subscription *sub,
> + List *subrelids, List
> *relids)
> ```
>
> The name CheckRefreshTableNotShared() is not suitable, because not sure the
> meaning "Shared". How about CheckRefreshTableNotInOtherSubscriptions?
>
> 03.
> ```
> + /*
> + * Tuples reach a partition through its ancestors, so a
> subscription
> + * tracking any of them keeps this relation populated
> too. A named
> + * relation's own entry belongs to this subscription and
> is skipped
> + * below by the srsubid test.
> + */
> + feeders = lappend_oid(get_partition_ancestors(relid),
> relid);
> ```
>
> get_partition_ancestors() seems to assume that the given relation has at
> least
> one parent, but the patch does not ensure. Maybe get_rel_relispartition()
> or
> similar functions can be used.
>
> 04.
> You missed to update meson.build file.
>
> 05.
> ```
> +$node_subscriber->append_conf('postgresql.conf',
> + "wal_retrieve_retry_interval = 1ms");
> ```
>
> Can you clarify the reason why the setting is required? Can we remove if
> not needed?
>
> Best regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
>