On Wed, Sep 2, 2026 at 10:00 PM Zhijie Hou (Fujitsu) <[email protected]> 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
