Re: Add hint message for check_log_destination()

On Mon, Jul 10, 2023 at 2:07 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Mon, 10 Jul 2023 09:04:42 +0800, Japin Li <japinli(at)hotmail(dot)com> wrote in
> >
> > On Sat, 08 Jul 2023 at 12:48, Michael Paquier <michael(at)paquier(dot)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