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_duration of %u ms.",
4822> ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured max_retention_duration of %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_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."));I'm not sure this is worth fixing, but anyway the proposed patch is
attached.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Reword-recently-introduced-messages.patchtext/x-patch; charset=us-asciiDownload
From 871fb2e5c9c27dcc6bfef9d8acd30966b738261b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 16 Sep 2025 10:56:34 +0900
Subject: [PATCH] Reword recently introduced messages
Three error messages recently introduced by commit 0d48d393d46 use
somewhat confusing wording with as in the sense of because. In
addition, one of them employed a rather obscure tense. This patch
rewords both for clarity.
---
src/backend/replication/logical/worker.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 9b5885d57cf..24005568330 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4790,7 +4790,7 @@ stop_conflict_info_retention(RetainDeadTuplesData *rdt_data)
ereport(LOG,
errmsg("logical replication worker for subscription \"%s\" has stopped retaining the information for detecting conflicts",
MySubscription->name),
- errdetail("Retention is stopped as the apply process is not advancing its xmin within the configured max_retention_duration of %u ms.",
+ errdetail("Retention is stopped because the apply process has not advanced its xmin into the configured max_retention_duration of %u ms.",
MySubscription->maxretention));
}
@@ -4819,9 +4819,9 @@ resume_conflict_info_retention(RetainDeadTuplesData *rdt_data)
errmsg("logical replication worker for subscription \"%s\" will resume retaining the information for detecting conflicts",
MySubscription->name),
MySubscription->maxretention
- ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within 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.",
MySubscription->maxretention)
- : 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."));
/*
* Restart the worker to let the launcher initialize
--
2.47.3
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_duration of %u ms.",
4822> ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured max_retention_duration of %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_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."?
--
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
Attachments:
v2-0001-Reword-recently-introduced-messages.patchapplication/octet-stream; name=v2-0001-Reword-recently-introduced-messages.patchDownload
From a0f3afbeb6d3ded067b1e776d9f122e460657770 Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Wed, 17 Sep 2025 11:10:16 +0800
Subject: [PATCH v2] Reword recently introduced messages
Three error messages recently introduced by commit 0d48d393d46 use
somewhat confusing wording with as in the sense of because. In
addition, one of them employed a rather obscure tense. This patch
rewords both for clarity.
---
src/backend/replication/logical/worker.c | 6 +++---
src/test/subscription/t/035_conflicts.pl | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 9b5885d57cf..e3520d6c80a 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4790,7 +4790,7 @@ stop_conflict_info_retention(RetainDeadTuplesData *rdt_data)
ereport(LOG,
errmsg("logical replication worker for subscription \"%s\" has stopped retaining the information for detecting conflicts",
MySubscription->name),
- errdetail("Retention is stopped as the apply process is not advancing its xmin within the configured max_retention_duration of %u ms.",
+ errdetail("Retention is stopped because the apply process could not advance its xmin within the configured max_retention_duration of %u ms.",
MySubscription->maxretention));
}
@@ -4819,9 +4819,9 @@ resume_conflict_info_retention(RetainDeadTuplesData *rdt_data)
errmsg("logical replication worker for subscription \"%s\" will resume retaining the information for detecting conflicts",
MySubscription->name),
MySubscription->maxretention
- ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured max_retention_duration of %u ms.",
+ ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured max_retention_duration of %u ms.",
MySubscription->maxretention)
- : 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."));
/*
* Restart the worker to let the launcher initialize
diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl
index a526986c4e4..2470ef94397 100644
--- a/src/test/subscription/t/035_conflicts.pl
+++ b/src/test/subscription/t/035_conflicts.pl
@@ -633,7 +633,7 @@ $node_B->reload;
# Confirm that the retention resumes
$node_A->wait_for_log(
qr/logical replication worker for subscription "tap_sub_a_b" will resume retaining the information for detecting conflicts
-.*DETAIL:.* Retention is re-enabled as max_retention_duration is set to unlimited.*/,
+.*DETAIL:.* Retention is re-enabled because max_retention_duration is set to unlimited.*/,
$log_offset);
ok( $node_A->poll_query_until(
--
2.51.0.windows.1
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_duration of %u ms.",
+ ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured 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_duration of %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 configured max_retention_duration of %u ms.", + ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured 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_duration of %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 configured max_retention_duration of %u ms.", + ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured 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_duration of %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 configured max_retention_duration of %u ms.", + ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured 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_duration of %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.