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
0001-Re-initialize-conflict-slot-xmin-when-a-database-new.patch
Description: 0001-Re-initialize-conflict-slot-xmin-when-a-database-new.patch
0004-Add-missing-trailing-periods-to-update_deleted-confl.patch
Description: 0004-Add-missing-trailing-periods-to-update_deleted-confl.patch
0003-Test-advancement-of-conflict-detection-slot-with-asy.patch
Description: 0003-Test-advancement-of-conflict-detection-slot-with-asy.patch
0002-Report-WAL-insert-position-in-primary-status-upd.patch
Description: 0002-Report-WAL-insert-position-in-primary-status-upd.patch
