Re: Add hint message for check_log_destination()

2023-07-13 Thread Japin Li

On Thu, 13 Jul 2023 at 16:19, Masahiko Sawada  wrote:
> On Tue, Jul 11, 2023 at 10:24 AM Japin Li  wrote:
>>
>>
>> On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada  wrote:
>> > On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
>> >  wrote:
>> >>
>> >> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li  wrote 
>> >> in
>> >> >
>> >> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier  
>> >> > wrote:
>> >> > > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
>> >> > >> +  appendStringInfoString(, "\"stderr\"");
>> >> > >> +#ifdef HAVE_SYSLOG
>> >> > >> +  appendStringInfoString(, ", \"syslog\"");
>> >> > >> +#endif
>> >> > >> +#ifdef WIN32
>> >> > >> +  appendStringInfoString(, ", 
>> >> > >> \"eventlog\"");
>> >> > >> +#endif
>> >> > >> +  appendStringInfoString(, ", \"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

>From 3f709effe14de81b84bbc441aa5884ece0a757da Mon Sep 17 00:00:00 2001
From: Japin Li 
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();
+			appendStringInfoString(, "stderr");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(, ", syslog");
+#endif
+#ifdef WIN32
+			appendStringInfoString(, ", eventlog");
+#endif
+			appendStringInfoString(, ", 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



Re: Add hint message for check_log_destination()

2023-07-13 Thread Masahiko Sawada
On Tue, Jul 11, 2023 at 10:24 AM Japin Li  wrote:
>
>
> On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada  wrote:
> > On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
> >  wrote:
> >>
> >> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li  wrote in
> >> >
> >> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier  
> >> > wrote:
> >> > > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
> >> > >> +  appendStringInfoString(, "\"stderr\"");
> >> > >> +#ifdef HAVE_SYSLOG
> >> > >> +  appendStringInfoString(, ", \"syslog\"");
> >> > >> +#endif
> >> > >> +#ifdef WIN32
> >> > >> +  appendStringInfoString(, ", \"eventlog\"");
> >> > >> +#endif
> >> > >> +  appendStringInfoString(, ", \"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




Re: Add hint message for check_log_destination()

2023-07-10 Thread Japin Li


On Mon, 10 Jul 2023 at 14:23, Masahiko Sawada  wrote:
> On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
>  wrote:
>>
>> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li  wrote in
>> >
>> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier  wrote:
>> > > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
>> > >> +  appendStringInfoString(, "\"stderr\"");
>> > >> +#ifdef HAVE_SYSLOG
>> > >> +  appendStringInfoString(, ", \"syslog\"");
>> > >> +#endif
>> > >> +#ifdef WIN32
>> > >> +  appendStringInfoString(, ", \"eventlog\"");
>> > >> +#endif
>> > >> +  appendStringInfoString(, ", \"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.




Re: Add hint message for check_log_destination()

2023-07-10 Thread Masahiko Sawada
On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
 wrote:
>
> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li  wrote in
> >
> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier  wrote:
> > > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
> > >> +  appendStringInfoString(, "\"stderr\"");
> > >> +#ifdef HAVE_SYSLOG
> > >> +  appendStringInfoString(, ", \"syslog\"");
> > >> +#endif
> > >> +#ifdef WIN32
> > >> +  appendStringInfoString(, ", \"eventlog\"");
> > >> +#endif
> > >> +  appendStringInfoString(, ", \"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




Re: Add hint message for check_log_destination()

2023-07-09 Thread Michael Paquier
On Mon, Jul 10, 2023 at 02:07:09PM +0900, Kyotaro Horiguchi wrote:
> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li  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


signature.asc
Description: PGP signature


Re: Add hint message for check_log_destination()

2023-07-09 Thread Kyotaro Horiguchi
At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li  wrote in 
> 
> On Sat, 08 Jul 2023 at 12:48, Michael Paquier  wrote:
> > On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
> >> +  appendStringInfoString(, "\"stderr\"");
> >> +#ifdef HAVE_SYSLOG
> >> +  appendStringInfoString(, ", \"syslog\"");
> >> +#endif
> >> +#ifdef WIN32
> >> +  appendStringInfoString(, ", \"eventlog\"");
> >> +#endif
> >> +  appendStringInfoString(, ", \"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




Re: Add hint message for check_log_destination()

2023-07-09 Thread Japin Li


On Sat, 08 Jul 2023 at 12:48, Michael Paquier  wrote:
> On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
>> +appendStringInfoString(, "\"stderr\"");
>> +#ifdef HAVE_SYSLOG
>> +appendStringInfoString(, ", \"syslog\"");
>> +#endif
>> +#ifdef WIN32
>> +appendStringInfoString(, ", \"eventlog\"");
>> +#endif
>> +appendStringInfoString(, ", \"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.




Re: Add hint message for check_log_destination()

2023-07-07 Thread Michael Paquier
On Fri, Jul 07, 2023 at 07:23:47PM +0800, Japin Li wrote:
> + appendStringInfoString(, "\"stderr\"");
> +#ifdef HAVE_SYSLOG
> + appendStringInfoString(, ", \"syslog\"");
> +#endif
> +#ifdef WIN32
> + appendStringInfoString(, ", \"eventlog\"");
> +#endif
> + appendStringInfoString(, ", \"csvlog\", and 
> \"jsonlog\"");

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


signature.asc
Description: PGP signature


Re: Add hint message for check_log_destination()

2023-07-07 Thread Japin Li

On Fri, 07 Jul 2023 at 16:21, Masahiko Sawada  wrote:
> On Fri, Jul 7, 2023 at 4:53 PM Japin Li  wrote:
>>
>>
>> On Fri, 07 Jul 2023 at 14:46, jian he  wrote:
>> > On Fri, Jul 7, 2023 at 1:06 PM Japin Li  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(, "\"stderr\"");
> +#ifdef HAVE_SYSLOG
> +   appendStringInfo(, ", \"syslog\"");
> +#endif
> +#ifdef WIN32
> +   appendStringInfo(, ", \"eventlog\"");
> +#endif
> +   appendStringInfo(, ", \"csvlog\", and \"jsonlog\"");
>
> I think using appendStringInfoString() is a bit more natural and faster.
>

Thanks for your review! Fixed as per your suggession.

>From e0338c5085e655f9a25f13cd5acea9a3110806fd Mon Sep 17 00:00:00 2001
From: Japin Li 
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();
+			appendStringInfoString(, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfoString(, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfoString(, ", \"eventlog\"");
+#endif
+			appendStringInfoString(, ", \"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


-- 
Regrads,
Japin Li.



Re: Add hint message for check_log_destination()

2023-07-07 Thread Masahiko Sawada
On Fri, Jul 7, 2023 at 4:53 PM Japin Li  wrote:
>
>
> On Fri, 07 Jul 2023 at 14:46, jian he  wrote:
> > On Fri, Jul 7, 2023 at 1:06 PM Japin Li  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(, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+   appendStringInfo(, ", \"syslog\"");
+#endif
+#ifdef WIN32
+   appendStringInfo(, ", \"eventlog\"");
+#endif
+   appendStringInfo(, ", \"csvlog\", and \"jsonlog\"");

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

Regards,

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




Re: Add hint message for check_log_destination()

2023-07-07 Thread Japin Li

On Fri, 07 Jul 2023 at 14:46, jian he  wrote:
> On Fri, Jul 7, 2023 at 1:06 PM Japin Li  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.

>From 715f9811717c9d27f6b2f41db639b18e6ba58625 Mon Sep 17 00:00:00 2001
From: Japin Li 
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();
+			appendStringInfo(, "\"stderr\"");
+#ifdef HAVE_SYSLOG
+			appendStringInfo(, ", \"syslog\"");
+#endif
+#ifdef WIN32
+			appendStringInfo(, ", \"eventlog\"");
+#endif
+			appendStringInfo(, ", \"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



Re: Add hint message for check_log_destination()

2023-07-07 Thread jian he
On Fri, Jul 7, 2023 at 1:06 PM Japin Li  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 ?




Add hint message for check_log_destination()

2023-07-06 Thread Japin Li

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.

>From 7d6b50c9deaba576cdc28e170bff541f630415f9 Mon Sep 17 00:00:00 2001
From: Japin Li 
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();
+			appendStringInfo(, "stderr, csvlog, jsonlog");
+#ifdef HAVE_SYSLOG
+			appendStringInfo(, ", syslog");
+#endif
+#ifdef WIN32
+			appendStringInfo(, ", 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