Follow-up review items for update_deleted

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.

2. Premature slot advancement with asynchronous commit

The retain_dead_tuples feature waits for concurrent transactions to be applied
before advancing conflict_detection_slot.xmin. It fetches the WAL write position
from the publisher and waits until the apply remote position passes it. This is
necessary to retain dead tuples, commit timestamps, and origins for conflict
detection (update_deleted, update_origin_differs, delete_origin_differs) when
applying those concurrent transactions.

However, when asynchronous commit is enabled on the publisher, concurrent
transactions do not update the WAL write position immediately. As a result, the
received WAL position may be earlier than intended (e.g., it may not reflect the
latest committed transaction). This can cause conflict_detection_slot.xmin to
advance prematurely, losing dead tuples needed for conflict detection when
applying subsequent asynchronously committed transactions.

The patch fixes this by reporting the end of the last inserted WAL record instead,
ensuring that the insert position covers every transaction that has already
committed and may have a commit timestamp.

The test to reproduce it is in 0003; since it adds a new injection point and
uses sleep(), which we may not commit, I kept it as a separate patch.

3

Missing trailing periods to update_deleted conflict DETAILs. Simple fix in 0004.

I will add open items for above.

--
Other items (for recording only)
--

The following items were also reported but don't seem worth changing at this
point. They can be revisited later if needed - sharing them here for reference.

4

With track_commit_timestamp off and retain_dead_tuples on,
FindDeletedTupleInLocalRel() returns false unconditionally and silently, while
dead tuples are still retained.

This is not considered as a bug, as the documentation clearly states that
update_deleted requires track_commit_timestamp to be enabled, and we emit a
WARNING when retain_dead_tuples is enabled without it. If needed, we could
improve this by stopping retention when track_commit_timestamp is disabled, but
disabling track_commit_timestamp while retain_dead_tuples is enabled seems like
an extremely unlikely user behavior. So would be better to wait for user
feedback before taking any action.

5

max_retention_duration does not work for a disabled subscription or a
keep-failing worker, meaning retention cannot be stopped using this option in
those cases.

This is documented behavior (and user can disable the retain_dead_tuples
manually):

> This option is effective only when retain_dead_tuples is enabled and the apply
> worker associated with the subscription is active.

To improve this, we would need to store apply worker timings in a shared hash
table and have the launcher check it. However, the complexity, shared memory
management and race conditions between the launcher and worker didn't seem worth
the effort for now. Leaving this as a future improvement.

Best Regards,
Zhijie Hou