On Tuesday, September 16, 2025 11:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Sep 16, 2025 at 8:17 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> wrote:
> >
> > I noticed that the recent commit 0d48d393d46 introduced the following
> > three messages:
> >
> > 4793> errdetail("Retention is stopped as the apply process is not
> > 4793> advancing its xmin within the configured max_retention_duration
> > 4793> of %u ms.",
> > 4822> ? errdetail("Retention is re-enabled as the apply process is
> > 4822> advancing its xmin within the configured max_retention_duration
> > 4822> of %u ms.",
> > 4824> : errdetail("Retention is re-enabled as max_retention_duration
> > 4824> is set to unlimited."));
> >
> > I think I saw other instances of this kind of as recently, and I
> > thought we had agreed to avoid this usage and prefer because instead,
> > but I lost track of where that discussion took place.
> >
> > Anyway, unlike some past uses, these ones are apparently confusing,
> > and I'd like to propose changing the wording to because.
> >
>
> Thanks for pointing this out. I checked the code and found the use of 'because'
> is preferred.
+1
>
> > In addition, I felt that the tense in the second message is not
> > immediately clear. If it is reasonable and keeps the correct sense,
> > I'd like to propose changing "is (not) advancing its xmin within" to
> > "has (not) advanced its xmin into".
> >
> > + errdetail("Retention is stopped because the apply process has not
> > + advanced its xmin into the configured max_retention_duration of %u
> > + ms.", ? errdetail("Retention is re-enabled because the apply process
> > + has advanced its xmin into the configured max_retention_duration of
> > + %u ms.",
> > + : errdetail("Retention is re-enabled because max_retention_duration
> > + is set to unlimited."));
> >
>
> In the above sentence, has advanced sounds like we have already advanced
> but that is not the case. Also, use of into looks odd to me.
> How about changing it to: "Retention is re-enabled because the apply process
> can advance its xmin within the configured max_retention_duration of %u
> ms."?
>
> Similarly for the first message, how about: "Retention is stopped because the
> apply process could not advance its xmin within the configured
> max_retention_duration of %u ms."?
I think the suggested message aligns better with the implementation.
I updated the message based on Horiguchi-San's revision. Additionally,
035_conflicts.pl has been modified, as it was waiting for the resume DETAIL
message and this message has now been updated.
Best Regards,
Hou zj
