RE: Reword messages using "as" instead of "because"

From: "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Cc: "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: Reword messages using "as" instead of "because"
Date: 2025-09-17 03:22:56
Message-ID: TY4PR01MB1690739EBFA0443A7A83028449417A@TY4PR01MB16907.jpnprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tuesday, September 16, 2025 11:56 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Sep 16, 2025 at 8:17 AM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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

Attachment Content-Type Size
v2-0001-Reword-recently-introduced-messages.patch application/octet-stream 2.9 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2025-09-17 03:25:09 Re: [PATCH] Add tests for Bitmapset
Previous Message Yugo Nagata 2025-09-17 02:28:14 pgbench: remove an unused function argument