On Tuesday, August 26, 2025 5:02 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Aug 26, 2025 at 12:15 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Mon, Aug 25, 2025 at 5:05 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > >
>
> Some comments on latest patch
Thanks for the comments!
> 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"
I slightly reworded it to "potentially resulting in incorrect conflict detection."
>
> 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.
I refactored this part based on some other comments, so these points
is addressed in the V66 patch set as well.
Best Regards,
Hou zj
