Add hint message for check_log_destination()

Started by Japin Liover 2 years ago13 messages

japinli@hotmail.com

over 2 years ago

1 attachment(s)

Hi, hackers

When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values. This
patch tries to add a hint message that provides available values for
log_destination. Any thoughts?

--
Regrads,
Japin Li.

Attachments:

v1-0001-Add-hint-message-for-log_destination.patchtext/x-diffDownload

From 7d6b50c9deaba576cdc28e170bff541f630415f9 Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Fri, 7 Jul 2023 12:59:09 +0800
Subject: [PATCH v1 1/1] Add hint message for log_destination

---
 src/backend/utils/error/elog.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..fdc6557a59 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,21 @@ check_log_destination(char **newval, void **extra, GucSource source)
 #endif
 		else
 		{
+			StringInfoData errhint;
+
+			initStringInfo(&errhint);
+			appendStringInfo(&errhint, "stderr, csvlog, jsonlog");
+#ifdef HAVE_SYSLOG
+			appendStringInfo(&errhint, ", syslog");
+#endif
+#ifdef WIN32
+			appendStringInfo(&errhint, ", eventlog");
+#endif
+
 			GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+			GUC_check_errhint("Available values: %s.", errhint.data);
 			pfree(rawstring);
+			pfree(errhint.data);
 			list_free(elemlist);
 			return false;
 		}
-- 
2.25.1

jian he

jian.universality@gmail.com

over 2 years ago

In reply to: Japin Li (#1)

Re: Add hint message for check_log_destination()

On Fri, Jul 7, 2023 at 1:06 PM Japin Li <japinli@hotmail.com> wrote:

Hi, hackers

When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values. This
patch tries to add a hint message that provides available values for
log_destination. Any thoughts?

--
Regrads,
Japin Li.

select * from pg_settings where name ~* 'log.*destin*' \gx

short_desc | Sets the destination for server log output.
extra_desc | Valid values are combinations of "stderr", "syslog",
"csvlog", "jsonlog", and "eventlog", depending on the platform.

you can just reuse extra_desc in the pg_settings (view) column ?

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: jian he (#2)

1 attachment(s)

Re: Add hint message for check_log_destination()

On Fri, 07 Jul 2023 at 14:46, jian he <jian.universality@gmail.com> wrote:

On Fri, Jul 7, 2023 at 1:06 PM Japin Li <japinli@hotmail.com> wrote:

Hi, hackers

When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values. This
patch tries to add a hint message that provides available values for
log_destination. Any thoughts?

select * from pg_settings where name ~* 'log.*destin*' \gx

short_desc | Sets the destination for server log output.
extra_desc | Valid values are combinations of "stderr", "syslog",
"csvlog", "jsonlog", and "eventlog", depending on the platform.

you can just reuse extra_desc in the pg_settings (view) column ?

Thanks for your review!

Yeah, the description of extra_desc is more accurate. Updated.

--
Regrads,
Japin Li.

Attachments:

v2-0001-Add-hint-message-for-check_log_destination.patchtext/x-diffDownload

From 715f9811717c9d27f6b2f41db639b18e6ba58625 Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Fri, 7 Jul 2023 15:48:53 +0800
Subject: [PATCH v2 1/1] Add hint message for check_log_destination

---
 src/backend/utils/error/elog.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..a32f9613be 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,22 @@ check_log_destination(char **newval, void **extra, GucSource source)
 #endif
 		else
 		{
+			StringInfoData errhint;
+
+			initStringInfo(&errhint);
+			appendStringInfo(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfo(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfo(&errhint, ", \"eventlog\"");
+#endif
+			appendStringInfo(&errhint, ", \"csvlog\", and \"jsonlog\"");
+
 			GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+			GUC_check_errhint("Valid values are combinations of %s.", errhint.data);
 			pfree(rawstring);
+			pfree(errhint.data);
 			list_free(elemlist);
 			return false;
 		}
-- 
2.25.1

Masahiko Sawada

sawada.mshk@gmail.com

over 2 years ago

In reply to: Japin Li (#3)

Re: Add hint message for check_log_destination()

On Fri, Jul 7, 2023 at 4:53 PM Japin Li <japinli@hotmail.com> wrote:

On Fri, 07 Jul 2023 at 14:46, jian he <jian.universality@gmail.com> wrote:

On Fri, Jul 7, 2023 at 1:06 PM Japin Li <japinli@hotmail.com> wrote:

Hi, hackers

When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values. This
patch tries to add a hint message that provides available values for
log_destination. Any thoughts?

+           appendStringInfo(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+           appendStringInfo(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+           appendStringInfo(&errhint, ", \"eventlog\"");
+#endif
+           appendStringInfo(&errhint, ", \"csvlog\", and \"jsonlog\"");

I think using appendStringInfoString() is a bit more natural and faster.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Masahiko Sawada (#4)

1 attachment(s)

Re: Add hint message for check_log_destination()

On Fri, 07 Jul 2023 at 16:21, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jul 7, 2023 at 4:53 PM Japin Li <japinli@hotmail.com> wrote:

On Fri, 07 Jul 2023 at 14:46, jian he <jian.universality@gmail.com> wrote:

On Fri, Jul 7, 2023 at 1:06 PM Japin Li <japinli@hotmail.com> wrote:

Hi, hackers

When I try to change log_destination using ALTER SYSTEM with the wrong value,
it complains of the "Unrecognized key word" without available values. This
patch tries to add a hint message that provides available values for
log_destination. Any thoughts?

+1
+           appendStringInfo(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+           appendStringInfo(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+           appendStringInfo(&errhint, ", \"eventlog\"");
+#endif
+           appendStringInfo(&errhint, ", \"csvlog\", and \"jsonlog\"");
I think using appendStringInfoString() is a bit more natural and faster.

Thanks for your review! Fixed as per your suggession.

Attachments:

v3-0001-Add-hint-message-for-check_log_destination.patchtext/x-diffDownload

From e0338c5085e655f9a25f13cd5acea9a3110806fd Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Fri, 7 Jul 2023 15:48:53 +0800
Subject: [PATCH v3 1/1] Add hint message for check_log_destination

---
 src/backend/utils/error/elog.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..a7545dcbd9 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,22 @@ check_log_destination(char **newval, void **extra, GucSource source)
 #endif
 		else
 		{
+			StringInfoData errhint;
+
+			initStringInfo(&errhint);
+			appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+			appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");
+
 			GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+			GUC_check_errhint("Valid values are combinations of %s.", errhint.data);
 			pfree(rawstring);
+			pfree(errhint.data);
 			list_free(elemlist);
 			return false;
 		}
-- 
2.25.1

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Japin Li (#5)

Re: Add hint message for check_log_destination()

On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:

+			appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+			appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");

Hmm. Is that OK as a translatable string?
--
Michael

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Michael Paquier (#6)

Re: Add hint message for check_log_destination()

On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:

+			appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+			appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");

Hmm. Is that OK as a translatable string?

Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

--
Regrads,
Japin Li.

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 2 years ago

In reply to: Japin Li (#7)

Re: Add hint message for check_log_destination()

At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli@hotmail.com> wrote in

On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
+			appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+			appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");
Hmm. Is that OK as a translatable string?
Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

At the very least, we can't generate comma-separated lists
programatically because punctuation marks vary across languages.

One potential approach could involve defining the message for every
potential combination, in full length.

Honestly, I'm not sold on the idea that we need to exhaust ourselves
providing an exhaustive list of usable keywords for users here. I
believe that it is unlikely that these keywords will be used in
different combinations each time without looking at the
documentation. On top of that, consider "csvlog" as an example, -- it
doesn't work as expected if logging_collector is off. Although this is
documented, we don't give any warnings at startup. This seems like a
bigger issue than the unusable keywords. (I don't mean to suggest to
fix this, as usual.)

In short, I think a simple message like '"xxx" cannot be used in this
build' should suffice for keywords defined but unusable, and we should
stick with "unknown" for the undefined ones.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Kyotaro Horiguchi (#8)

Re: Add hint message for check_log_destination()

On Mon, Jul 10, 2023 at 02:07:09PM +0900, Kyotaro Horiguchi wrote:

At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli@hotmail.com> wrote in

Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

Per the documentation:
https://www.postgresql.org/docs/devel/nls-programmer.html#NLS-GUIDELINES

Now, if you want to look at the shape of the messages, you could also
run something like a `make init-po` and look at the messages generated
in a .pot file.

Honestly, I'm not sold on the idea that we need to exhaust ourselves
providing an exhaustive list of usable keywords for users here. I
believe that it is unlikely that these keywords will be used in
different combinations each time without looking at the
documentation. On top of that, consider "csvlog" as an example, -- it
doesn't work as expected if logging_collector is off. Although this is
documented, we don't give any warnings at startup. This seems like a
bigger issue than the unusable keywords. (I don't mean to suggest to
fix this, as usual.)

In short, I think a simple message like '"xxx" cannot be used in this
build' should suffice for keywords defined but unusable, and we should
stick with "unknown" for the undefined ones.

Which is roughly what the existing GUC_check_errdetail() does as well,
but you indeed lose a bit of context because the option wanted is not
built. I am not convinced that there is something to change here.
--
Michael

#10

Masahiko Sawada

sawada.mshk@gmail.com

over 2 years ago

In reply to: Kyotaro Horiguchi (#8)

Re: Add hint message for check_log_destination()

On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:

At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli@hotmail.com> wrote in
On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
+                  appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+                  appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+                  appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+                  appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");
Hmm. Is that OK as a translatable string?

It seems okay to me but needs to be checked.

Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

At the very least, we can't generate comma-separated lists
programatically because punctuation marks vary across languages.

One potential approach could involve defining the message for every
potential combination, in full length.

Don't we generate a comma-separated list for an error hint of an enum
parameter? For example, to generate the following error hint:

=# alter system set client_min_messages = 'aaa';
ERROR: invalid value for parameter "client_min_messages": "aaa"
HINT: Available values: debug5, debug4, debug3, debug2, debug1, log,
notice, warning, error.

we use the comma-separated generated by config_enum_get_options() and
do ereport() like:

ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));

IMO log_destination is a string GUC parameter but its value is the
list of enums. So it makes sense to me to add a hint message like what
we do for enum parameters in case where the user mistypes a wrong
value. I'm not sure why the proposed patch needs to quote the usable
values, though. A similar type of GUC parameter is debug_io_direct.
But I'm not sure we need a hint message for it too as it's a developer
option.

On top of that, consider "csvlog" as an example, -- it
doesn't work as expected if logging_collector is off. Although this is
documented, we don't give any warnings at startup. This seems like a
bigger issue than the unusable keywords. (I don't mean to suggest to
fix this, as usual.)

Yes, but I think it's a separate problem.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

#11

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Masahiko Sawada (#10)

Re: Add hint message for check_log_destination()

On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli@hotmail.com> wrote in
On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
+                  appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+                  appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+                  appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+                  appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");
Hmm. Is that OK as a translatable string?
It seems okay to me but needs to be checked.

Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

At the very least, we can't generate comma-separated lists
programatically because punctuation marks vary across languages.

One potential approach could involve defining the message for every
potential combination, in full length.

Don't we generate a comma-separated list for an error hint of an enum
parameter? For example, to generate the following error hint:

=# alter system set client_min_messages = 'aaa';
ERROR: invalid value for parameter "client_min_messages": "aaa"
HINT: Available values: debug5, debug4, debug3, debug2, debug1, log,
notice, warning, error.

we use the comma-separated generated by config_enum_get_options() and
do ereport() like:

ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));

IMO log_destination is a string GUC parameter but its value is the
list of enums. So it makes sense to me to add a hint message like what
we do for enum parameters in case where the user mistypes a wrong
value. I'm not sure why the proposed patch needs to quote the usable
values, though.

I borrowed the description from pg_settings extra_desc. In my first patch,
I used the hint message line enum parameter, however, since it might be a
combination of multiple log destinations, so, I update the patch using
extra_desc.

--
Regrads,
Japin Li.

#12

Masahiko Sawada

sawada.mshk@gmail.com

over 2 years ago

In reply to: Japin Li (#11)

Re: Add hint message for check_log_destination()

On Tue, Jul 11, 2023 at 10:24 AM Japin Li <japinli@hotmail.com> wrote:

On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli@hotmail.com> wrote in
On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
+                  appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+                  appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+                  appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+                  appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");
Hmm. Is that OK as a translatable string?
It seems okay to me but needs to be checked.

Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

At the very least, we can't generate comma-separated lists
programatically because punctuation marks vary across languages.

One potential approach could involve defining the message for every
potential combination, in full length.

Don't we generate a comma-separated list for an error hint of an enum
parameter? For example, to generate the following error hint:

=# alter system set client_min_messages = 'aaa';
ERROR: invalid value for parameter "client_min_messages": "aaa"
HINT: Available values: debug5, debug4, debug3, debug2, debug1, log,
notice, warning, error.

we use the comma-separated generated by config_enum_get_options() and
do ereport() like:

ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));
IMO log_destination is a string GUC parameter but its value is the
list of enums. So it makes sense to me to add a hint message like what
we do for enum parameters in case where the user mistypes a wrong
value. I'm not sure why the proposed patch needs to quote the usable
values, though.

I borrowed the description from pg_settings extra_desc. In my first patch,
I used the hint message line enum parameter, however, since it might be a
combination of multiple log destinations, so, I update the patch using
extra_desc.

I agree to use description from pg_settings extra_desc, but it seems
to be better not to quote each available value like we do for enum
parameter cases. That is, the hint message would be like:

=# alter system set log_destination to 'xxx';
ERROR: invalid value for parameter "log_destination": "xxx"
DETAIL: Unrecognized key word: "xxx".
HINT: Valid values are combinations of stderr, syslog, csvlog, and jsonlog.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

#13

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Masahiko Sawada (#12)

1 attachment(s)

Re: Add hint message for check_log_destination()

On Thu, 13 Jul 2023 at 16:19, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jul 11, 2023 at 10:24 AM Japin Li <japinli@hotmail.com> wrote:
On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli@hotmail.com> wrote in
On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
+                  appendStringInfoString(&errhint, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+                  appendStringInfoString(&errhint, ", \"syslog\"");
+#endif
+#ifdef WIN32
+                  appendStringInfoString(&errhint, ", \"eventlog\"");
+#endif
+                  appendStringInfoString(&errhint, ", \"csvlog\", and \"jsonlog\"");
Hmm. Is that OK as a translatable string?
It seems okay to me but needs to be checked.

Sorry for the late reply! I'm not sure. How can I know whether it is translatable?

At the very least, we can't generate comma-separated lists
programatically because punctuation marks vary across languages.

One potential approach could involve defining the message for every
potential combination, in full length.

Don't we generate a comma-separated list for an error hint of an enum
parameter? For example, to generate the following error hint:

=# alter system set client_min_messages = 'aaa';
ERROR: invalid value for parameter "client_min_messages": "aaa"
HINT: Available values: debug5, debug4, debug3, debug2, debug1, log,
notice, warning, error.

we use the comma-separated generated by config_enum_get_options() and
do ereport() like:

ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value for parameter \"%s\": \"%s\"",
name, value),
hintmsg ? errhint("%s", _(hintmsg)) : 0));
IMO log_destination is a string GUC parameter but its value is the
list of enums. So it makes sense to me to add a hint message like what
we do for enum parameters in case where the user mistypes a wrong
value. I'm not sure why the proposed patch needs to quote the usable
values, though.

I borrowed the description from pg_settings extra_desc. In my first patch,
I used the hint message line enum parameter, however, since it might be a
combination of multiple log destinations, so, I update the patch using
extra_desc.
I agree to use description from pg_settings extra_desc, but it seems
to be better not to quote each available value like we do for enum
parameter cases. That is, the hint message would be like:

=# alter system set log_destination to 'xxx';
ERROR: invalid value for parameter "log_destination": "xxx"
DETAIL: Unrecognized key word: "xxx".
HINT: Valid values are combinations of stderr, syslog, csvlog, and jsonlog.

Agreed. Fixed as per your suggestions.

--
Regrads,
Japin Li

Attachments:

v4-0001-Add-hint-message-for-check_log_destination.patchtext/x-diffDownload

From 3f709effe14de81b84bbc441aa5884ece0a757da Mon Sep 17 00:00:00 2001
From: Japin Li <japinli@hotmail.com>
Date: Fri, 14 Jul 2023 09:26:01 +0800
Subject: [PATCH v4 1/1] Add hint message for check_log_destination

---
 src/backend/utils/error/elog.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..dccbabf40a 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -2228,8 +2228,22 @@ check_log_destination(char **newval, void **extra, GucSource source)
 #endif
 		else
 		{
+			StringInfoData errhint;
+
+			initStringInfo(&errhint);
+			appendStringInfoString(&errhint, "stderr");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(&errhint, ", syslog");
+#endif
+#ifdef WIN32
+			appendStringInfoString(&errhint, ", eventlog");
+#endif
+			appendStringInfoString(&errhint, ", csvlog, and jsonlog");
+
 			GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+			GUC_check_errhint("Valid values are combinations of %s.", errhint.data);
 			pfree(rawstring);
+			pfree(errhint.data);
 			list_free(elemlist);
 			return false;
 		}
-- 
2.41.0