Обсуждение: Reword messages using "as" instead of "because"

Hello.

I noticed that the recent commit 0d48d393d46 introduced the following
three messages:

4793> errdetail("Retention is stopped as the apply process is not advancing its xmin within the configured
max_retention_durationof %u ms.",
 
4822> ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured
max_retention_durationof %u ms.",
 
4824> : errdetail("Retention is re-enabled as max_retention_duration 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.

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_durationof %u ms.",
 
+ ? errdetail("Retention is re-enabled because the apply process has advanced its xmin into the configured
max_retention_durationof %u ms.",
 
+ : errdetail("Retention is re-enabled because max_retention_duration is set to unlimited."));

I'm not sure this is worth fixing, but anyway the proposed patch is
attached.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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 advancing its xmin within the configured
max_retention_durationof %u ms.", 
> 4822> ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured
max_retention_durationof %u ms.", 
> 4824> : errdetail("Retention is re-enabled as max_retention_duration 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.

> 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_durationof %u ms.", 
> + ? errdetail("Retention is re-enabled because the apply process has advanced its xmin into the configured
max_retention_durationof %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."?

--
With Regards,
Amit Kapila.

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

On Wed, Sep 17, 2025 at 8:53 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> >
> > 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.
>

LGTM. Horiguchi-San, do let me know if you have suggestions here. I am
planning to push this tomorrow.

--
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> LGTM. Horiguchi-San, do let me know if you have suggestions here. I am
> planning to push this tomorrow.

+1 for the first change, but for this:

-            ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured
max_retention_durationof %u ms.", 
+            ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured
max_retention_durationof %u ms.", 

would it be better to say

    "Retention is re-enabled because the apply process was able to advance its xmin within the configured
max_retention_durationof %u ms." 

If this isn't a statement that xmin has already been advanced, then
I'm not sure quite what it means.

Also here:

-            : errdetail("Retention is re-enabled as max_retention_duration is set to unlimited."));
+            : errdetail("Retention is re-enabled because max_retention_duration is set to unlimited."));

I think maybe what is meant is

    "Retention is re-enabled because max_retention_duration has been set to unlimited."

or you could say "has been changed to".  We'd never have got to this
if max_retention_duration had been unlimited all along, correct?

Passing by mere grammatical issues ... the patch shows that only one
of these three cases is reached in the regression tests.  Is that
a coverage gap that we should worry about?

            regards, tom lane

On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > LGTM. Horiguchi-San, do let me know if you have suggestions here. I am
> > planning to push this tomorrow.
>
> +1 for the first change, but for this:
>
> -                       ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the
configuredmax_retention_duration of %u ms.", 
> +                       ? errdetail("Retention is re-enabled because the apply process can advance its xmin within
theconfigured max_retention_duration of %u ms.", 
>
> would it be better to say
>
>         "Retention is re-enabled because the apply process was able to advance its xmin within the configured
max_retention_durationof %u ms." 
>
> If this isn't a statement that xmin has already been advanced, then
> I'm not sure quite what it means.
>

xmin is not yet advanced. In this state, we ensured that the
subscriber has caught up with the publisher and now the apply worker
can start maintaining/advancing its xmin.

> Also here:
>
> -                       : errdetail("Retention is re-enabled as max_retention_duration is set to unlimited."));
> +                       : errdetail("Retention is re-enabled because max_retention_duration is set to unlimited."));
>
> I think maybe what is meant is
>
>         "Retention is re-enabled because max_retention_duration has been set to unlimited."
>
> or you could say "has been changed to".  We'd never have got to this
> if max_retention_duration had been unlimited all along, correct?
>

Right, so will use your version.

> Passing by mere grammatical issues ... the patch shows that only one
> of these three cases is reached in the regression tests.  Is that
> a coverage gap that we should worry about?
>

We thought adding for one of these cases is sufficient to avoid
increasing the test timing further. These are time sensitive tests as
apply-worker on subscriber is dependent on actions of publisher, so we
need wait logic.  Otherwise, it seems doable to once again stop the
retention and resume due to a non-zero value of
max_retention_duration.

--
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1 for the first change, but for this:
>>
>> -                       ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the
configuredmax_retention_duration of %u ms.", 
>> +                       ? errdetail("Retention is re-enabled because the apply process can advance its xmin within
theconfigured max_retention_duration of %u ms.", 
>>
>> would it be better to say
>>
>> "Retention is re-enabled because the apply process was able to advance its xmin within the configured
max_retention_durationof %u ms." 

> xmin is not yet advanced. In this state, we ensured that the
> subscriber has caught up with the publisher and now the apply worker
> can start maintaining/advancing its xmin.

Hm, so what has max_retention_duration got to do with it?  That
is, should the message just read

    "Retention is re-enabled because the apply process can advance its xmin."
or better
    "Retention is re-enabled because the apply process has caught up with the publisher."

This now reminds me of a point that I meant to make in my previous
reply and forgot: this whole business of "advancing xmin" is
implementation jargon.  Can we rephrase it to make sense to a
user who has no idea what xmin is?  I think the concepts of being
caught up to the publisher or falling behind the publisher should
be pretty clear to most users, but none of these proposed messages
are that.

>> Passing by mere grammatical issues ... the patch shows that only one
>> of these three cases is reached in the regression tests.  Is that
>> a coverage gap that we should worry about?

> We thought adding for one of these cases is sufficient to avoid
> increasing the test timing further. These are time sensitive tests as
> apply-worker on subscriber is dependent on actions of publisher, so we
> need wait logic.

Hmm, if it's time-sensitive then yeah, building a stable test
case would be very hard.

            regards, tom lane

On Thu, Sep 18, 2025 at 10:22 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> +1 for the first change, but for this:
> >>
> >> -                       ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the
configuredmax_retention_duration of %u ms.", 
> >> +                       ? errdetail("Retention is re-enabled because the apply process can advance its xmin within
theconfigured max_retention_duration of %u ms.", 
> >>
> >> would it be better to say
> >>
> >> "Retention is re-enabled because the apply process was able to advance its xmin within the configured
max_retention_durationof %u ms." 
>
> > xmin is not yet advanced. In this state, we ensured that the
> > subscriber has caught up with the publisher and now the apply worker
> > can start maintaining/advancing its xmin.
>
> Hm, so what has max_retention_duration got to do with it?
>

It is the duration used to avoid subscriber being too much behind
publisher (and hence leading to retaining dead tuples for conflict
detection for a very long time). If the apply worker on the subscriber
is not caught up for this (max_retention_duration) duration then we
stop retaining dead tuples. Similarly, when the apply worker is able
to catch up before max_retention_duration is elapsed, we will resume
retention.

>
  That
> is, should the message just read
>
>         "Retention is re-enabled because the apply process can advance its xmin."
> or better
>         "Retention is re-enabled because the apply process has caught up with the publisher."
>
> This now reminds me of a point that I meant to make in my previous
> reply and forgot: this whole business of "advancing xmin" is
> implementation jargon.
>

Yeah, this sounds clear but shall we consider using
max_retention_duration like: "Retention is re-enabled because the
apply process has caught up with the publisher within the configured
max_retention_duration.". We can have a single message if we don't
want to specify the value of max_retention_duration or simply skip
adding max_retention_duration.

--
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> Yeah, this sounds clear but shall we consider using
> max_retention_duration like: "Retention is re-enabled because the
> apply process has caught up with the publisher within the configured
> max_retention_duration.". We can have a single message if we don't
> want to specify the value of max_retention_duration or simply skip
> adding max_retention_duration.

That wording sounds good to me.  I think you could leave out
the mention of max_retention_duration, but I won't fight if
people prefer to include it.

            regards, tom lane

On Thu, Sep 18, 2025 at 5:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Yeah, this sounds clear but shall we consider using
> > max_retention_duration like: "Retention is re-enabled because the
> > apply process has caught up with the publisher within the configured
> > max_retention_duration.". We can have a single message if we don't
> > want to specify the value of max_retention_duration or simply skip
> > adding max_retention_duration.
>
> That wording sounds good to me.  I think you could leave out
> the mention of max_retention_duration, but I won't fight if
> people prefer to include it.
>

We have a similar message for stop retention. I feel it would be good
to mention that as a reason, so users can increase it. I could think
of two alternatives for stop message based on above suggestion:
"Retention is stopped because the apply process has not caught up with
the publisher within the configured max_retention_duration."
"Retention is stopped because the apply process could not catch up
with the publisher within the configured max_retention_duration."

Do you have any preference? The first one resembles a similar resume
message and second is probably what I would have used if there was no
corresponding resume message.

--
With Regards,
Amit Kapila.

Amit Kapila <amit.kapila16@gmail.com> writes:
> We have a similar message for stop retention. I feel it would be good
> to mention that as a reason, so users can increase it. I could think
> of two alternatives for stop message based on above suggestion:
> "Retention is stopped because the apply process has not caught up with
> the publisher within the configured max_retention_duration."
> "Retention is stopped because the apply process could not catch up
> with the publisher within the configured max_retention_duration."

> Do you have any preference?

I think "has not" is clearer, or maybe you should say "did not catch
up with..."  Either way, that sounds like a pure statement of fact
whereas "could not" has some overtones of assigning blame.

            regards, tom lane

On Fri, Sep 19, 2025 at 9:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > We have a similar message for stop retention. I feel it would be good
> > to mention that as a reason, so users can increase it. I could think
> > of two alternatives for stop message based on above suggestion:
> > "Retention is stopped because the apply process has not caught up with
> > the publisher within the configured max_retention_duration."
> > "Retention is stopped because the apply process could not catch up
> > with the publisher within the configured max_retention_duration."
>
> > Do you have any preference?
>
> I think "has not" is clearer, or maybe you should say "did not catch
> up with..."  Either way, that sounds like a pure statement of fact
> whereas "could not" has some overtones of assigning blame.
>

Thanks for your inputs. I've pushed after making discussed changes.

--
With Regards,
Amit Kapila.