GUC names in messages

On Wed, Nov 1, 2023 at 8:02 PM Peter Smith <smithpb2250@gmail.com> wrote:
...

I had intended to make a patch to address the inconsistency, but
couldn't decide which of those styles was the preferred one.

Then I worried this could be the tip of the iceberg -- GUC names occur
in many other error messages where they are sometimes quoted and
sometimes not quoted:
e.g. Not quoted -- errhint("You might need to run fewer transactions
at a time or increase max_connections.")));
e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
\"wal_segment_size\"")));

Ideally, they should all look the same everywhere, shouldn't they?

One idea to achieve consistency might be to always substitute GUC
names using a macro.

#define GUC_NAME(s) ("\"" s "\"")

ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s must be at least twice %s",
GUC_NAME("max_wal_size"),
GUC_NAME("wal_segment_size"))));

Thoughts?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On 1 Nov 2023, at 10:02, Peter Smith <smithpb2250@gmail.com> wrote:

GUC_check_errdetail("effective_io_concurrency must be set to 0 on
platforms that lack posix_fadvise().");
src/backend/commands/variable.c:
GUC_check_errdetail("maintenance_io_concurrency must be set to 0 on
platforms that lack posix_fadvise().");

These should be substituted to reduce the number of distinct messages that need
to be translated. I wouldn't be surprised if more like these have slipped
through.

I had intended to make a patch to address the inconsistency, but
couldn't decide which of those styles was the preferred one.

Given the variety in the codebase I don't think there is a preferred one.

Then I worried this could be the tip of the iceberg

All good rabbit-holes uncovered during hacking are.. =)

Ideally, they should all look the same everywhere, shouldn't they?

Having a policy would be good, having one which is known and enforced is even
better (like how we are consistent around error messages based on our Error
Message Style Guide).

--
Daniel Gustafsson

On 1 Nov 2023, at 10:22, Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, Nov 1, 2023 at 8:02 PM Peter Smith <smithpb2250@gmail.com> wrote:
...

I had intended to make a patch to address the inconsistency, but
couldn't decide which of those styles was the preferred one.

Then I worried this could be the tip of the iceberg -- GUC names occur
in many other error messages where they are sometimes quoted and
sometimes not quoted:
e.g. Not quoted -- errhint("You might need to run fewer transactions
at a time or increase max_connections.")));
e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
\"wal_segment_size\"")));

Ideally, they should all look the same everywhere, shouldn't they?

One idea to achieve consistency might be to always substitute GUC
names using a macro.

#define GUC_NAME(s) ("\"" s "\"")

ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s must be at least twice %s",
GUC_NAME("max_wal_size"),
GUC_NAME("wal_segment_size"))));

Something like this might make translations harder since the remaining string
leaves little context about the message. We already have that today to some
extent (so it might not be an issue), and it might be doable to automatically
add translator comments, but it's something to consider.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:

On 1 Nov 2023, at 10:22, Peter Smith <smithpb2250@gmail.com> wrote:

One idea to achieve consistency might be to always substitute GUC
names using a macro.

#define GUC_NAME(s) ("\"" s "\"")

ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s must be at least twice %s",
GUC_NAME("max_wal_size"),
GUC_NAME("wal_segment_size"))));

Something like this might make translations harder since the remaining string
leaves little context about the message. We already have that today to some
extent (so it might not be an issue), and it might be doable to automatically
add translator comments, but it's something to consider.

Our error message style guidelines say not to assemble messages out
of separate parts, because it makes translation difficult. Originally
we applied that rule to GUC names mentioned in messages as well.
Awhile ago the translation team decided that that made for too many
duplicative translations, so they'd be willing to compromise on
substituting GUC names. That's only been changed in a haphazard
fashion though, mostly in cases where there actually were duplicative
messages that could be merged this way. And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s. So that's why we have
a mishmash right now.

I'm not enamored of the GUC_NAME idea suggested above. I don't
think it buys anything, and what it does do is make *every single
one* of our GUC-mentioning messages wrong. I think if we want to
standardize here, we should standardize on something that's
already pretty common in the code base.

Another problem with the idea as depicted above is that it
mistakenly assumes that "..." is the correct quoting method
in all languages. You could make GUC_NAME be a pure no-op
macro and continue to put quoting in the translatable string
where it belongs, but then the macro brings even less value.

regards, tom lane

On 01.11.23 10:25, Tom Lane wrote:

And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s. So that's why we have
a mishmash right now.

I'm leaning toward not quoting GUC names. The quoting is needed in
places where the value can be arbitrary, to avoid potential confusion.
But the GUC names are well-known, and we wouldn't add confusing GUC
names like "table" or "not found" in the future.

On Wed, 2023-11-01 at 16:12 -0400, Peter Eisentraut wrote:

On 01.11.23 10:25, Tom Lane wrote:

And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s. So that's why we have
a mishmash right now.

I'm leaning toward not quoting GUC names. The quoting is needed in
places where the value can be arbitrary, to avoid potential confusion.
But the GUC names are well-known, and we wouldn't add confusing GUC
names like "table" or "not found" in the future.

I agree for names with underscores in them. But I think that quoting
is necessary for names like "timezone" or "datestyle" that might be
mistaken for normal words. My personal preference is to always quote
GUC names, but I think it is OK not to quote GOCs whose name are
clearly not natural language words.

Yours,
Laurenz Albe

On Thu, Nov 2, 2023 at 1:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Daniel Gustafsson <daniel@yesql.se> writes:

On 1 Nov 2023, at 10:22, Peter Smith <smithpb2250@gmail.com> wrote:

One idea to achieve consistency might be to always substitute GUC
names using a macro.

#define GUC_NAME(s) ("\"" s "\"")

ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s must be at least twice %s",
GUC_NAME("max_wal_size"),
GUC_NAME("wal_segment_size"))));

Something like this might make translations harder since the remaining string
leaves little context about the message. We already have that today to some
extent (so it might not be an issue), and it might be doable to automatically
add translator comments, but it's something to consider.

Our error message style guidelines say not to assemble messages out
of separate parts, because it makes translation difficult. Originally
we applied that rule to GUC names mentioned in messages as well.
Awhile ago the translation team decided that that made for too many
duplicative translations, so they'd be willing to compromise on
substituting GUC names. That's only been changed in a haphazard
fashion though, mostly in cases where there actually were duplicative
messages that could be merged this way. And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s. So that's why we have
a mishmash right now.

I'm not enamored of the GUC_NAME idea suggested above. I don't
think it buys anything, and what it does do is make *every single
one* of our GUC-mentioning messages wrong. I think if we want to
standardize here, we should standardize on something that's
already pretty common in the code base.

Thanks to everybody for the feedback received so far.

Perhaps as a first step, I can try to quantify the GUC name styles
already in the source code. The numbers might help decide how to
proceed

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Nov 01, 2023 at 09:46:52PM +0100, Laurenz Albe wrote:

I agree for names with underscores in them. But I think that quoting
is necessary for names like "timezone" or "datestyle" that might be
mistaken for normal words. My personal preference is to always quote
GUC names, but I think it is OK not to quote GOCs whose name are
clearly not natural language words.

+1, IMHO quoting GUC names makes it abundantly clear that they are special
identifiers. In de4d456, we quoted the role names in a bunch of messages.
We didn't quote the attribute/option names, but those are in all-caps, so
they already stand out nicely.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 2023-Nov-01, Nathan Bossart wrote:

On Wed, Nov 01, 2023 at 09:46:52PM +0100, Laurenz Albe wrote:

I agree for names with underscores in them. But I think that quoting
is necessary for names like "timezone" or "datestyle" that might be
mistaken for normal words. My personal preference is to always quote
GUC names, but I think it is OK not to quote GOCs whose name are
clearly not natural language words.

+1, IMHO quoting GUC names makes it abundantly clear that they are special
identifiers. In de4d456, we quoted the role names in a bunch of messages.
We didn't quote the attribute/option names, but those are in all-caps, so
they already stand out nicely.

I like this, and I propose we codify it in the message style guide. How
about this? We can start looking at code changes to make once we decide
we agree with this.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La verdad no siempre es bonita, pero el hambre de ella sí"

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2023-Nov-01, Nathan Bossart wrote:

+1, IMHO quoting GUC names makes it abundantly clear that they are special
identifiers. In de4d456, we quoted the role names in a bunch of messages.
We didn't quote the attribute/option names, but those are in all-caps, so
they already stand out nicely.

I like this, and I propose we codify it in the message style guide. How
about this? We can start looking at code changes to make once we decide
we agree with this.

WFM.

regards, tom lane

On Tue, Nov 07, 2023 at 10:33:03AM +0100, Alvaro Herrera wrote:

On 2023-Nov-01, Nathan Bossart wrote:

+1, IMHO quoting GUC names makes it abundantly clear that they are special
identifiers. In de4d456, we quoted the role names in a bunch of messages.
We didn't quote the attribute/option names, but those are in all-caps, so
they already stand out nicely.

I like this, and I propose we codify it in the message style guide. How
about this? We can start looking at code changes to make once we decide
we agree with this.

+   <para>
+    In messages containing configuration variable names, quotes are
+    not necessary when the names are visibly not English natural words, such
+    as when they have underscores or are all-uppercase.  Otherwise, quotes
+    must be added.  Do include double-quotes in a message where an arbitrary
+    variable name is to be expanded.
+   </para>

І'd vote for quoting all GUC names, if for no other reason than "visibly
not English natural words" feels a bit open to interpretation. But this
seems like it's on the right track, so I won't argue too strongly if I'm
the only holdout.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

FWIW, I am halfway through doing regex checking of the PG16 source for
all GUC names in messages to see what current styles are in use today.

Not sure if those numbers will influence the decision.

I hope I can post my findings today or tomorrow.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Nov 8, 2023 at 7:40 AM Peter Smith <smithpb2250@gmail.com> wrote:

FWIW, I am halfway through doing regex checking of the PG16 source for
all GUC names in messages to see what current styles are in use today.

Not sure if those numbers will influence the decision.

I hope I can post my findings today or tomorrow.

Here are my findings from the current PG16 source messages.

I used a regex search:
".*GUCNAME

to find how each GUCNAME is used in the messages in *.c files.

The GUC names are taken from the guc_tables.c code, so they are
grouped accordingly below.

~TOTALS:

messages where GUC names are QUOTED:
- bool = 11
- int = 11
- real = 0
- string = 10
- enum = 7
TOTAL = 39

messages where GUC names are NOT QUOTED:
- bool = 14
- int = 60
- real = 0
- string = 59
- enum = 31
TOTAL = 164

~~~

Details are in the attached file. PSA.

I've categorised them as being currently QUOTED, NOT QUOTED, and NONE
(most are not used in any messages).

Notice that NOT QUOTED is the far more common pattern, so my vote
would be just to standardise on making everything this way. I know
there was some concern raised about ambiguous words like "timezone"
and "datestyle" etc but in practice, those are rare. Also, those GUCs
are different in that they are written as camel-case (e.g.
"DateStyle") in the guc_tables.c, so if they were also written
camel-case in the messages that could remove ambiguities with normal
words. YMMV.

Anyway, I will await a verdict about what to do.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Nov 2, 2023 at 1:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

...

Our error message style guidelines say not to assemble messages out
of separate parts, because it makes translation difficult. Originally
we applied that rule to GUC names mentioned in messages as well.
Awhile ago the translation team decided that that made for too many
duplicative translations, so they'd be willing to compromise on
substituting GUC names. That's only been changed in a haphazard
fashion though, mostly in cases where there actually were duplicative
messages that could be merged this way. And there's never been any
real clarity about whether to quote GUC names, though certainly we're
more likely to quote anything injected with %s. So that's why we have
a mishmash right now.

Right. While looking at all the messages I observed a number of them
having almost the same (but not quite the same) wording:

For example,

errhint("Consider increasing the configuration parameter \"max_wal_size\".")));
errhint("You might need to increase %s.", "max_locks_per_transaction")));
errhint("You might need to increase %s.", "max_pred_locks_per_transaction")));
errmsg("could not find free replication state, increase
max_replication_slots")));
hint ? errhint("You might need to increase %s.", "max_slot_wal_keep_size") : 0);
errhint("You may need to increase max_worker_processes.")));
errhint("Consider increasing configuration parameter
\"max_worker_processes\".")));
errhint("Consider increasing the configuration parameter
\"max_worker_processes\".")));
errhint("You might need to increase %s.", "max_worker_processes")));
errhint("You may need to increase max_worker_processes.")));
errhint("You might need to increase %s.", "max_logical_replication_workers")));

~

The most common pattern there is "You might need to increase %s.".

Here is a patch to modify those other similar variations so they share
that common wording.

PSA.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

At Thu, 9 Nov 2023 12:55:44 +1100, Peter Smith <smithpb2250@gmail.com> wrote in

The most common pattern there is "You might need to increase %s.".

..

Here is a patch to modify those other similar variations so they share
that common wording.

PSA.

I'm uncertain whether the phrases "Consider doing something" and "You
might need to do something" are precisely interchangeable. However,
(for me..) it appears that either phrase could be applied for all
messages that the patch touches.

In short, I'm fine with the patch.

By the way, I was left scratching my head after seeing the following
message.

ereport(PANIC,
(errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
- errmsg("could not find free replication state, increase max_replication_slots")));

Being told to increase max_replication_slots in a PANIC
message feels somewhat off to me. Just looking at the message, it
seems unconvincing to increase "slots" because there is a lack of
"state". So, I poked around in the code and found the following
comment:

ReplicationOriginShmemSize(void)
{
...
/*
* XXX: max_replication_slots is arguably the wrong thing to use, as here
* we keep the replay state of *remote* transactions. But for now it seems
* sufficient to reuse it, rather than introduce a separate GUC.
*/

I haven't read the related code, but if my understanding based on this
comment is correct, wouldn't it mean that a lack of storage space for
the state at the location outputting the message indicates a bug in
the program, not a user configuration error? In other words, isn't
this message something that at least shouldn't be a user-facing
message, and might it be more appropriate to use an Assert instead?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2023-Nov-09, Peter Smith wrote:

Notice that NOT QUOTED is the far more common pattern, so my vote
would be just to standardise on making everything this way. I know
there was some concern raised about ambiguous words like "timezone"
and "datestyle" etc but in practice, those are rare. Also, those GUCs
are different in that they are written as camel-case (e.g.
"DateStyle") in the guc_tables.c, so if they were also written
camel-case in the messages that could remove ambiguities with normal
words. YMMV.

Well, I think camel-casing is also a sufficient differentiator for these
identifiers not being English words. We'd need to ensure they're always
written that way, when not quoted. However, in cases where arbitrary
values are expanded, I don't know that they would be expanded that way,
so I would still go for quoting in that case.

There's also a few that are not camel-cased nor have any underscores --
looking at postgresql.conf.sample, we have "port", "bonjour", "ssl",
"fsync", "geqo", "jit", "autovacuum", "xmlbinary", "xmloption". (We also
have "include", but I doubt that's ever used in an error message). But
actually, there's more: every reloption is a candidate, and there we
have "fillfactor", "autosummarize", "fastupdate", "buffering". So if we
want to make generic advice on how to deal with option names in error
messages, I think the wording on conditional quoting I proposed should
go in (adding CamelCase as a reason not to quote), and then we can fix
the code to match. Looking at your list, I think the changes to make
are not too numerous.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Nadie está tan esclavizado como el que se cree libre no siéndolo" (Goethe)

On Thu, Nov 9, 2023 at 10:04 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Nov-09, Peter Smith wrote:

Notice that NOT QUOTED is the far more common pattern, so my vote
would be just to standardise on making everything this way. I know
there was some concern raised about ambiguous words like "timezone"
and "datestyle" etc but in practice, those are rare. Also, those GUCs
are different in that they are written as camel-case (e.g.
"DateStyle") in the guc_tables.c, so if they were also written
camel-case in the messages that could remove ambiguities with normal
words. YMMV.

Well, I think camel-casing is also a sufficient differentiator for these
identifiers not being English words. We'd need to ensure they're always
written that way, when not quoted. However, in cases where arbitrary
values are expanded, I don't know that they would be expanded that way,
so I would still go for quoting in that case.

There's also a few that are not camel-cased nor have any underscores --
looking at postgresql.conf.sample, we have "port", "bonjour", "ssl",
"fsync", "geqo", "jit", "autovacuum", "xmlbinary", "xmloption". (We also
have "include", but I doubt that's ever used in an error message). But
actually, there's more: every reloption is a candidate, and there we
have "fillfactor", "autosummarize", "fastupdate", "buffering". So if we
want to make generic advice on how to deal with option names in error
messages, I think the wording on conditional quoting I proposed should
go in (adding CamelCase as a reason not to quote), and then we can fix
the code to match. Looking at your list, I think the changes to make
are not too numerous.

Sorry for my delay in getting back to this thread.

PSA a patch for this work.

There may be some changes I've missed, but hopefully, this is a nudge
in the right direction.

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Thu, Nov 23, 2023 at 06:27:04PM +1100, Peter Smith wrote:

There may be some changes I've missed, but hopefully, this is a nudge
in the right direction.

Thanks for spending some time on that.

    <para>
+    In messages containing configuration variable names, do not include quotes
+    when the names are visibly not English natural words, such as when they
+    have underscores or are all-uppercase or have mixed case. Otherwise, quotes
+    must be added.  Do include quotes in a message where an arbitrary variable
+    name is to be expanded.
+   </para>

That seems to describe clearly the consensus reached on the thread
(quotes for GUCs that are single terms, no quotes for names that are
obviously parameters).

In terms of messages that have predictible names, 0002 moves in the
needle in the right direction. There seem to be more:
src/backend/postmaster/bgworker.c: errhint("Consider increasing the
configuration parameter \"max_worker_processes\".")));
contrib/pg_prewarm/autoprewarm.c: errhint("Consider increasing
configuration parameter \"max_worker_processes\".")));

Things like parse_and_validate_value() and set_config_option_ext()
include log strings about GUC and these use quotes. Could these areas
be made smarter with a routine to check if quotes are applied
automatically when we have a "simple" GUC name, aka I guess made of
only lower-case characters? This could be done with a islower() on
the string name, for instance.
--
Michael

On 2023-Nov-24, Michael Paquier wrote:

On Thu, Nov 23, 2023 at 06:27:04PM +1100, Peter Smith wrote:

There may be some changes I've missed, but hopefully, this is a nudge
in the right direction.

Thanks for spending some time on that.

+1

<para>
+    In messages containing configuration variable names, do not include quotes
+    when the names are visibly not English natural words, such as when they
+    have underscores or are all-uppercase or have mixed case. Otherwise, quotes
+    must be added.  Do include quotes in a message where an arbitrary variable
+    name is to be expanded.
+   </para>

That seems to describe clearly the consensus reached on the thread
(quotes for GUCs that are single terms, no quotes for names that are
obviously parameters).

Yeah, this is pretty much the patch I proposed earlier.

In terms of messages that have predictible names, 0002 moves in the
needle in the right direction. There seem to be more:
src/backend/postmaster/bgworker.c: errhint("Consider increasing the
configuration parameter \"max_worker_processes\".")));
contrib/pg_prewarm/autoprewarm.c: errhint("Consider increasing
configuration parameter \"max_worker_processes\".")));

Yeah. Also, these could be changed to have the GUC name outside the
message proper, which would reduce the total number of messages. (But
care must be given to the word "the" there.)

Things like parse_and_validate_value() and set_config_option_ext()
include log strings about GUC and these use quotes. Could these areas
be made smarter with a routine to check if quotes are applied
automatically when we have a "simple" GUC name, aka I guess made of
only lower-case characters? This could be done with a islower() on
the string name, for instance.

I think we could leave these improvements for a second round. They
don't need to hold back the improvement we already have.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick." (Andrew Sullivan)
/messages/by-id/20050809113420.GD2768@phlogiston.dyndns.org