Subject: GUC names in messages

From: Peter Smith <[email-masked]>
Date: Wed, 1 Nov 2023 20:02:01 +1100

Hi hackers,

While reviewing another patch I noticed how the GUCs are
inconsistently named within the GUC_check_errdetail messages:

======

below, the GUC name is embedded but not quoted:

src/backend/access/transam/xlogprefetcher.c:
GUC_check_errdetail("recovery_prefetch is not supported on platforms
that lack posix_fadvise().");
src/backend/access/transam/xlogrecovery.c:
GUC_check_errdetail("recovery_target_timeline is not a valid
number.");
src/backend/commands/variable.c:
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().");
src/backend/port/sysv_shmem.c:
GUC_check_errdetail("huge_page_size must be 0 on this platform.");
src/backend/port/win32_shmem.c:
GUC_check_errdetail("huge_page_size must be 0 on this platform.");
src/backend/replication/syncrep.c:
GUC_check_errdetail("synchronous_standby_names parser failed");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported on this
platform.");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported for WAL because
XLOG_BLCKSZ is too small");
src/backend/storage/file/fd.c:
GUC_check_errdetail("debug_io_direct is not supported for data because
BLCKSZ is too small");
src/backend/tcop/postgres.c:
GUC_check_errdetail("client_connection_check_interval must be set to 0
on this platform.");

~~~

below, the GUC name is embedded and double-quoted:

src/backend/commands/vacuum.c:
GUC_check_errdetail("\"vacuum_buffer_usage_limit\" must be 0 or
between %d kB and %d kB",
src/backend/commands/variable.c:
GUC_check_errdetail("Conflicting \"datestyle\" specifications.");
src/backend/storage/buffer/localbuf.c:
GUC_check_errdetail("\"temp_buffers\" cannot be changed after any
temporary tables have been accessed in the session.");
src/backend/tcop/postgres.c:
GUC_check_errdetail("\"max_stack_depth\" must not exceed %ldkB.",
src/backend/tcop/postgres.c:        GUC_check_errdetail("Cannot enable
parameter when \"log_statement_stats\" is true.");
src/backend/tcop/postgres.c:        GUC_check_errdetail("Cannot enable
\"log_statement_stats\" when "

~~~

below, the GUC name is substituted but not quoted:

src/backend/access/table/tableamapi.c:      GUC_check_errdetail("%s
cannot be empty.",
src/backend/access/table/tableamapi.c:      GUC_check_errdetail("%s is
too long (maximum %d characters).",

~~~

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?

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

---

From: Peter Smith <[email-masked]>
Date: Wed, 1 Nov 2023 20:22:30 +1100

On Wed, Nov 1, 2023 at 8:02 PM Peter Smith <[email-masked]> wrote:
...
> 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

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 1 Nov 2023 10:23:22 +0100

> On 1 Nov 2023, at 10:02, Peter Smith <[email-masked]> wrote:

> 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.

> 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

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 1 Nov 2023 10:59:21 +0100

> On 1 Nov 2023, at 10:22, Peter Smith <[email-masked]> wrote:
> 

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 01 Nov 2023 10:25:17 -0400

> 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

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 1 Nov 2023 16:12:20 -0400

On 01.11.23 10:25, Tom Lane wrote:
> And there's never been any

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.

---

From: Laurenz Albe <[email-masked]>
Date: Wed, 01 Nov 2023 21:46:52 +0100

> 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