Re: Conflict detection for update_deleted in logical replication

On Tue, Aug 26, 2025 at 12:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Aug 25, 2025 at 5:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >

Some comments on latest patch
0001:

1.
+ <para>
+ Note that setting a non-zero value for this option could lead to
+ information for conflict detection being removed prematurely,
+ potentially missing some conflict detections.
+ </para>

We can improve this wording by saying "potentially incorrectly
detecting some conflict"

2.
@@ -1175,6 +1198,8 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
bool update_failover = false;
bool update_two_phase = false;
bool check_pub_rdt = false;
+ bool ineffective_maxconflretention = false;
+ bool update_maxretention = false;

For making variable names more consistent, better to change
'ineffective_maxconflretention' to 'ineffective_maxretention' so that
this will be more consistent with 'update_maxretention'

3.
+/*
+ * Report a NOTICE to inform users that max_conflict_retention_duration is
+ * ineffective when retain_dead_tuples is disabled for a subscription. An ERROR
+ * is not issued because setting max_conflict_retention_duration
causes no harm,
+ * even when it is ineffective.
+ */
+static void
+notify_ineffective_max_retention(bool update_maxretention)
+{
+ ereport(NOTICE,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ update_maxretention
+ ? errmsg("max_conflict_retention_duration has no effect when
retain_dead_tuples is disabled")
+ : errmsg("disabling retain_dead_tuples will render
max_conflict_retention_duration ineffective"));
}

I really don't like to make a function for a single ereport, even if
this is being called from multiple places.

--
Regards,
Dilip Kumar
Google