Re: Follow-up review items for update_deleted

On Wed, Sep 2, 2026 at 10:00 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Hi,
>
> Nisha helped perform a thorough review of the update_deleted feature using
> Claude and shared the findings with me off-list. After some analysis and
> discussion, we identified a few items worth addressing (fixes are
> attached (0001-0004)). Please see the details below:
>
> 1. Slot xmin backward movement with multiple subscriptions
>
> Since the retain_dead_tuples feature maintains one replication slot for all
> subscriptions, if two subscriptions are created at different times, the later
> one may cause slot.xmin to move backwards.
>
> For example, consider two subscriptions for databases A and B. The oldest XID
> in database A is 700, while the oldest XID in database B is 500 (due to a
> long-running transaction in that database). If subscription A is created first,
> conflict_detection_slot.xmin advances to 700. When subscription B is later
> created, it sees that the oldest XID in its own database is 500 ? which is older
> than the current slot.xmin. This currently causes an Assert failure in the apply
> worker. If the Assert is removed, it would allow slot.xmin to move backwards.
>
> Moving slot.xmin backwards is actually correct behavior for subscription of
> database B: the long-running transaction in database B is a candidate that could
> generate dead tuples needed for update_deleted conflict detection, so slot.xmin
> should not advance beyond that XID. However, instead of letting the worker
> handle this (which could delay the slot.xmin update), we should have the
> launcher detect the new subscription and adjust the slot directly.
>
> The patch fix this by tracking the set of databases with actively-retaining
> subscriptions in the launcher, and when a database newly appears in the set,
> re-initialize the slot's xmin to the cluster-wide safe decoding horizon before
> launching any workers. The horizon accounts for all running transactions
> cluster-wide, so it is a safe seed for every database.
>
> See 0001 for the fix and test.
>

Hi Hou-san,

Thanks for the fix patches.

I’ve reviewed and tested the Issue-1 fix (patch-001), and it LGTM. I
didn’t find any critical issues with the fix.

Just one test comment on [035_conflicts.pl:699]:

$node_B->wait_for_subscription_sync($node_A, $subname_BA2);

If the subscription is in a db other than postgres, we need to pass
the db name here. Otherwise, it connects to the default postgres db,
and the test could pass without actually checking the intended
subscription.

I think this should be:

$node_B->wait_for_subscription_sync($node_A, $subname_BA2, 'dbb');

--
Thanks,
Nisha