Re: Conflict detection for update_deleted in logical replication

[Amit Kapila]

On Fri, Sep 12, 2025 at 8:55 AM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> I agree. Here is a V73 patch that will restart the worker if the retention
> resumes. I also addressed other comments posted by Amit[1].
>

Few comments:
============
* In adjust_xid_advance_interval(), for the case when retention is not
active, we still cap the interval by wal_receiver_status_interval. Is
that required or do we let it go till 3 minutes? We can add a new else
if loop to keep the code clear, if you agree with this.

*
+ /*
+ * Resume retention immediately if required. (See
+ * should_resume_retention_immediately() for details).
+ */
+ if (should_resume_retention_immediately(rdt_data, status_received))
+ rdt_data->phase = RDT_RESUME_CONFLICT_INFO_RETENTION;

Is this optimization for the case when we are in stop_phase or after
stop_phase and someone has changed maxretention to 0? If so, I don't
think it is worth optimizing for such a rare case at the cost of
making code difficult to understand.

Apart from this, I have changed a few comments in the attached.

-- 
With Regards,
Amit Kapila.

diff --git a/src/backend/replication/logical/worker.c 
b/src/backend/replication/logical/worker.c
index 1274192de7e..7d1ed506622 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4363,10 +4363,7 @@ maybe_advance_nonremovable_xid(RetainDeadTuplesData 
*rdt_data,
        if (!can_advance_nonremovable_xid(rdt_data))
                return;
 
-       /*
-        * Resume retention immediately if required. (See
-        * should_resume_retention_immediately() for details).
-        */
+       /* Resume dead_tuples retention immediately if required. */
        if (should_resume_retention_immediately(rdt_data, status_received))
                rdt_data->phase = RDT_RESUME_CONFLICT_INFO_RETENTION;
 
@@ -4549,8 +4546,8 @@ wait_for_publisher_status(RetainDeadTuplesData *rdt_data,
        {
                /*
                 * Stop retention if not yet. Otherwise, reset to the initial 
phase to
-                * retry all phases. This is required to recalculate the 
current wait
-                * time and resume retention if the time falls within
+                * retry resuming retention. This reset is required to 
recalculate the
+                * current wait time and resume retention if the time falls 
within
                 * max_retention_duration.
                 */
                if (MySubscription->retentionactive)
@@ -4683,8 +4680,8 @@ wait_for_local_flush(RetainDeadTuplesData *rdt_data)
        {
                /*
                 * Stop retention if not yet. Otherwise, reset to the initial 
phase to
-                * retry all phases. This is required to recalculate the 
current wait
-                * time and resume retention if the time falls within
+                * retry resuming retention. This reset is required to 
recalculate the
+                * current wait time and resume retention if the time falls 
within
                 * max_retention_duration.
                 */
                if (MySubscription->retentionactive)
@@ -4763,9 +4760,7 @@ wait_for_local_flush(RetainDeadTuplesData *rdt_data)
  * Check whether conflict information retention should be stopped due to
  * exceeding the maximum wait time (max_retention_duration).
  *
- * If retention should be stopped, transition to the
- * RDT_STOP_CONFLICT_INFO_RETENTION phase and return true. Otherwise, return
- * false.
+ * If retention should be stopped, return true. Otherwise, return false.
  */
 static bool
 should_stop_conflict_info_retention(RetainDeadTuplesData *rdt_data)