A question about wording in messages
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.
I found other three uses of "we" in backend.
client sent proto_version=%d but we only support protocol %d or lower
client sent proto_version=%d but we only support protocol %d or higher
System allows %d, we need at least %d.
This is a little different from the first one. In the three above,
"we" suggests "The developers and maybe the PostgreSQL program".
Is it the right word choice as error messages? I'm not confident on
the precise wording, but I think something like the following are
appropriate here.
This controls the maximum distance to read ahead in the WAL to prefetch referenced data blocks.
client sent proto_version=%d but only protocols %d or lower are supported
client sent proto_version=%d but only protocols %d or higher are supported
System allows %d, at least %d needed.
Thoughts?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.
+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe
"Maximum distance to read ahead in WAL to prefetch data blocks."
regards, tom lane
At Tue, 13 Sep 2022 22:38:46 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe"Maximum distance to read ahead in WAL to prefetch data blocks."
It seems to sufficiently work for average users and rather easy to
read, but it looks a short description.
wal_decode_buffer_size has the following descriptions.
Short: Buffer size for reading ahead in the WAL during recovery.
Extra: This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
So, taking the middle of them, how about the following?
Short: Buffer size for reading ahead in the WAL during recovery.
Extra: This controls the maximum distance to read ahead in WAL to prefetch data blocks."
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Sep 14, 2022 at 7:45 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.I found other three uses of "we" in backend.
client sent proto_version=%d but we only support protocol %d or lower
client sent proto_version=%d but we only support protocol %d or higher
How about just replacing 'we' with 'server'?
System allows %d, we need at least %d.
Another possibility could be: "System allows %d, but at least %d are required."
This is a little different from the first one. In the three above,
"we" suggests "The developers and maybe the PostgreSQL program".Is it the right word choice as error messages? I'm not confident on
the precise wording, but I think something like the following are
appropriate here.This controls the maximum distance to read ahead in the WAL to prefetch referenced data blocks.
client sent proto_version=%d but only protocols %d or lower are supported
client sent proto_version=%d but only protocols %d or higher are supported
System allows %d, at least %d needed.
This could be another way to rewrite. Let us see if others have an
opinion on this.
--
With Regards,
Amit Kapila.
On Wed, Sep 14, 2022 at 2:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe"Maximum distance to read ahead in WAL to prefetch data blocks."
+1
For "we", I must have been distracted by code comment style. For the
extra useless verbiage, it's common for GUC description to begin "This
control/affects/blah" like that, but I agree it's useless noise.
For the other cases, Amit's suggestion of 'server' seems sensible to me.
At Fri, 16 Sep 2022 12:10:05 +1200, Thomas Munro <thomas.munro@gmail.com> wrote in
On Wed, Sep 14, 2022 at 2:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe"Maximum distance to read ahead in WAL to prefetch data blocks."
+1
For "we", I must have been distracted by code comment style. For the
extra useless verbiage, it's common for GUC description to begin "This
control/affects/blah" like that, but I agree it's useless noise.For the other cases, Amit's suggestion of 'server' seems sensible to me.
Thaks for the opinion. I'm fine with that, too.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
v2-0001-Get-rid-of-we-from-messages.patchtext/x-patch; charset=us-asciiDownload
From 31df0a224d7d4d6cb619534f24940c4c3571cfd9 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Fri, 16 Sep 2022 15:07:21 +0900
Subject: [PATCH v2] Get rid of "we" from messages
It's not our usual style to use "we" in error messages. Get rid of
that usages so that the messages sound usual. On the way fixing them,
simplify a verbose message.
---
src/backend/replication/pgoutput/pgoutput.c | 4 ++--
src/backend/storage/file/fd.c | 2 +-
src/backend/utils/misc/guc_tables.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 62e0ffecd8..9c8faece10 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -441,13 +441,13 @@ pgoutput_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
if (data->protocol_version > LOGICALREP_PROTO_MAX_VERSION_NUM)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("client sent proto_version=%d but we only support protocol %d or lower",
+ errmsg("client sent proto_version=%d but server only supports protocol %d or lower",
data->protocol_version, LOGICALREP_PROTO_MAX_VERSION_NUM)));
if (data->protocol_version < LOGICALREP_PROTO_MIN_VERSION_NUM)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("client sent proto_version=%d but we only support protocol %d or higher",
+ errmsg("client sent proto_version=%d but server only supports protocol %d or higher",
data->protocol_version, LOGICALREP_PROTO_MIN_VERSION_NUM)));
if (data->publication_names == NIL)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 20c3741aa1..073dab2be5 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -978,7 +978,7 @@ set_max_safe_fds(void)
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_RESOURCES),
errmsg("insufficient file descriptors available to start server process"),
- errdetail("System allows %d, we need at least %d.",
+ errdetail("System allows %d, server needs at least %d.",
max_safe_fds + NUM_RESERVED_FDS,
FD_MINFREE + NUM_RESERVED_FDS)));
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 550e95056c..5c340d471d 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2591,7 +2591,7 @@ struct config_int ConfigureNamesInt[] =
{
{"wal_decode_buffer_size", PGC_POSTMASTER, WAL_RECOVERY,
gettext_noop("Buffer size for reading ahead in the WAL during recovery."),
- gettext_noop("This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks."),
+ gettext_noop("Maximum distance to read ahead in WAL to prefetch data blocks."),
GUC_UNIT_BYTE
},
&wal_decode_buffer_size,
--
2.31.1
On Fri, Sep 16, 2022 at 11:46 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Fri, 16 Sep 2022 12:10:05 +1200, Thomas Munro <thomas.munro@gmail.com> wrote in
On Wed, Sep 14, 2022 at 2:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe"Maximum distance to read ahead in WAL to prefetch data blocks."
+1
For "we", I must have been distracted by code comment style. For the
extra useless verbiage, it's common for GUC description to begin "This
control/affects/blah" like that, but I agree it's useless noise.For the other cases, Amit's suggestion of 'server' seems sensible to me.
Thaks for the opinion. I'm fine with that, too.
So, the change related to wal_decode_buffer_size needs to be
backpatched to 15 whereas other message changes will be HEAD only, am
I correct?
--
With Regards,
Amit Kapila.
At Fri, 16 Sep 2022 12:29:52 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
So, the change related to wal_decode_buffer_size needs to be
backpatched to 15 whereas other message changes will be HEAD only, am
I correct?
Has 15 closed the entry? IMHO I supposed that all changes are applied
back(?) to 15.
regardes.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Sep 16, 2022 at 2:07 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Fri, 16 Sep 2022 12:29:52 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
So, the change related to wal_decode_buffer_size needs to be
backpatched to 15 whereas other message changes will be HEAD only, am
I correct?Has 15 closed the entry? IMHO I supposed that all changes are applied
back(?) to 15.
We only want to commit the changes to 15 (a) if those fixes a problem
introduced in 15, or (b) those are for a bug fix. I think the error
message improvements fall into none of those categories, we can map it
to (b) but I feel those are an improvement in the current messages and
don't seem critical to me.
--
With Regards,
Amit Kapila.
On Fri, Sep 16, 2022 at 12:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe"Maximum distance to read ahead in WAL to prefetch data blocks."
+1
For "we", I must have been distracted by code comment style. For the
extra useless verbiage, it's common for GUC description to begin "This
control/affects/blah" like that, but I agree it's useless noise.For the other cases, Amit's suggestion of 'server' seems sensible to me.
Thaks for the opinion. I'm fine with that, too.
So, the change related to wal_decode_buffer_size needs to be
backpatched to 15 whereas other message changes will be HEAD only, am
I correct?
I would like to pursue as per above unless there is more feedback on this.
--
With Regards,
Amit Kapila.
On 2022-Sep-14, Kyotaro Horiguchi wrote:
At Tue, 13 Sep 2022 22:38:46 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
I saw the following message recently modified.
This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks.
Maybe the "we" means "PostgreSQL program and you" but I see it
somewhat out of place.+1, I saw that today and thought it was outside our usual style.
The whole thing is awfully verbose for a GUC description, too.
Maybe"Maximum distance to read ahead in WAL to prefetch data blocks."
I failed to notice this issue. I agree it's unusual and +1 for changing it.
It seems to sufficiently work for average users and rather easy to
read, but it looks a short description.
So, taking the middle of them, how about the following?
Short: Buffer size for reading ahead in the WAL during recovery.
Extra: This controls the maximum distance to read ahead in WAL to prefetch data blocks."
But why do we care that it's short? We don't need it to be long .. we
only need it to explain what it needs to explain.
After spending way too much time editing this line, I ended up with
exactly what Tom proposed, so +1 for his version. I think "This
controls" adds nothing very useful, and we don't have it anywhere else,
except tcp_keepalives_count from where I also propose to remove it.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
Attachments:
0001-fix-some-GUC-strings.patchtext/x-diff; charset=us-asciiDownload
From 5b8bf15ed5d9f1a21150039da33a557f027640d4 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 20 Sep 2022 18:19:34 +0200
Subject: [PATCH] fix some GUC strings
---
src/backend/utils/misc/guc_tables.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 550e95056c..ab3140ff3a 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2591,7 +2591,7 @@ struct config_int ConfigureNamesInt[] =
{
{"wal_decode_buffer_size", PGC_POSTMASTER, WAL_RECOVERY,
gettext_noop("Buffer size for reading ahead in the WAL during recovery."),
- gettext_noop("This controls the maximum distance we can read ahead in the WAL to prefetch referenced data blocks."),
+ gettext_noop("Maximum distance to read ahead in the WAL to prefetch referenced data blocks."),
GUC_UNIT_BYTE
},
&wal_decode_buffer_size,
@@ -3248,7 +3248,7 @@ struct config_int ConfigureNamesInt[] =
{
{"tcp_keepalives_count", PGC_USERSET, CONN_AUTH_TCP,
gettext_noop("Maximum number of TCP keepalive retransmits."),
- gettext_noop("This controls the number of consecutive keepalive retransmits that can be "
+ gettext_noop("Number of consecutive keepalive retransmits that can be "
"lost before a connection is considered dead. A value of 0 uses the "
"system default."),
},
--
2.30.2
On 2022-Sep-16, Amit Kapila wrote:
We only want to commit the changes to 15 (a) if those fixes a problem
introduced in 15, or (b) those are for a bug fix. I think the error
message improvements fall into none of those categories, we can map it
to (b) but I feel those are an improvement in the current messages and
don't seem critical to me.
IMO at least the GUC one does fix a problem related to the wording of a
user-visible message, which also flows into the translations. I prefer
to have that one fixed it in 15 also. The other messages (errors) don't
seem very interesting because they're not as visible, so I don't care if
those are not backpatched.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"This is a foot just waiting to be shot" (Andrew Dunstan)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
After spending way too much time editing this line, I ended up with
exactly what Tom proposed, so +1 for his version. I think "This
controls" adds nothing very useful, and we don't have it anywhere else,
except tcp_keepalives_count from where I also propose to remove it.
LGTM.
regards, tom lane