Re: Per-table resync for logical replication subscriptions

Dear Hayato,

Thank you, v8 is attached.

> Here we does not acquire a lock for the root table if the publication
publishes
> only leaves. So subscriptions can be created after
> CheckRefreshTableNotInOtherSubscriptions(). I considered a race condition
due to
> this with LLM: See attached patch.

Confirmed, and your reproducer runs as written once
utils/injection_point.h is included in subscriptioncmds.c, which your patch
does not add. On v7 it fails the way you intended:

I considered the other obvious fix, taking AccessExclusiveLock on
pg_subscription_rel as AlterSubscription_refresh() does, and did not use it,
for two reasons. It is the lock you had me weaken to RowExclusiveLock in the
v4 round, because this command waits for locks on user relations while
holding
it and that closes a deadlock cycle, which AlterSubscription_refresh()
avoids
by only ever holding AccessShareLock on user relations. And it would be held
across the tablesync slot drops, so an unreachable publisher would block
every
subscription's state updates cluster-wide rather than one partition
hierarchy.

> Also, I found the test could fail like:
> got: 'tab_inh_c=s,tab_inh_p=i'
> expected: 'tab_inh_c=r,tab_inh_p=i'
> It tried to ensure the tab_inh_c was synchronized, right? Should accept
's'

Right on both counts, and thank you for catching it. The intent is that the
child was not reset, and wait_for_subscription_sync() waits for
srsubstate NOT IN ('r', 's'), so a table it has returned for may still be
SYNCDONE; disabling the subscription then freezes it there, since the
promotion
to READY is the apply worker's job.

Looking for that pattern found four more assertions with the same flaw, so
v8
fixes five rather than one:

- three that compared a state to 'r' where the point was that the table had
not been reset; they accept 'r' or 's' now, and the one that checks a
table
is untouched across the command compares it against the value captured
before instead, which is both immune to this and a stronger statement;
- the reported one, now matched with qr/^tab_inh_c=[rs],tab_inh_p=i$/;
- one that counted relations with srsubstate <> 'r' and expected zero, which
a SYNCDONE table would also have failed. It counts srsubstate = 'i'
instead, which is what "nothing was reset" actually means.

I checked that the loosened assertions still reject what they are there for:
tab_inh_c=i (child wrongly reset) and tab_inh_p=r (parent not reset) are
both
still refused.

Best regards,
Cagri Biroglu

On Mon, Sep 7, 2026 at 10:53 AM Hayato Kuroda (Fujitsu) <
kuroda(dot)hayato(at)fujitsu(dot)com> wrote:

> Dear Cagri,
>
> Thanks for the patch. I have comments for v7.
>
> ```
> + /*
> + * The relation set is now known to be fed by this subscription
> alone, and
> + * this subscription has no running workers, so nothing that
> replicates
> + * into these relations is still holding them. Take the level the
> + * truncate needs.
> + */
> + foreach_oid(relid, relids)
> + LockRelationOid(relid, AccessExclusiveLock);
> ```
>
> Here we does not acquire a lock for the root table if the publication
> publishes
> only leaves. So subscriptions can be created after
> CheckRefreshTableNotInOtherSubscriptions(). I considered a race condition
> due to
> this with LLM: See attached patch.
>
> Also, I found the test could fail like:
>
> ```
> [14:44:37.892](0.014s) not ok 35 - only the named inheritance parent is
> reset
> [14:44:37.893](0.000s) # Failed test 'only the named inheritance parent
> is reset'
> # at /home/hayato/postgres/src/test/subscription/t/039_refresh_table.pl
> line 407.
> [14:44:37.893](0.000s) # got: 'tab_inh_c=s,tab_inh_p=i'
> # expected: 'tab_inh_c=r,tab_inh_p=i'
> ```
>
> It tried to ensure the tab_inh_c was synchronized, right? Should accept 's'
>
> Best regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
>