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