Re: Conflict detection for update_deleted in logical replication - Mailing list pgsql-hackers

On Mon, Aug 25, 2025 at 10:06 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Attach the V65 patch set which addressed above and
> Shveta's comments[1].
>

A few comments on 0001:
1.
- if (opts.retaindeadtuples)
- CheckSubDeadTupleRetention(true, !sub->enabled, NOTICE);
+ CheckSubDeadTupleRetention(true, !sub->enabled, NOTICE,
+    opts.retaindeadtuples,
+    retention_active, false,
+    sub->maxconflretention);

  /*
  * Notify the launcher to manage the replication slot for
@@ -1434,6 +1487,20 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,

  check_pub_rdt = opts.retaindeadtuples;
  retain_dead_tuples = opts.retaindeadtuples;
+
+ ineffective_maxconflretention = (!opts.retaindeadtuples &&
+ sub->maxconflretention);

Why can't we handle this special ineffective_maxconflretention case
inside CheckSubDeadTupleRetention? If so, then we can directly give
the NOTICE in case of SUBOPT_MAX_CONFLICT_RETENTION_DURATION without
having a separate notify_ineffective_max_retention() function.

2.
- if (sub->retaindeadtuples && can_advance_xmin)
+ if (sub->retaindeadtuples && sub->retentionactive &&
+ can_advance_xmin)

This coding pattern looks odd, you can have one condition per line.

3. Are we setting retention_inactive in launcher.c to true ever?

4.
this is because the launcher assigns
+ * the initial oldest_nonremovable_xid after the apply worker updates the
+ * catalog (see resume_conflict_info_retention).

I don't see resume_conflict_info_retention in 0001, so I couldn't make
sense of this part of the comment.

--
With Regards,
Amit Kapila.