Question -- why is there no errhint_internal function?
Hi,
For each of the logging functions (see elog.c) there is an associated
XXX_internal function which is equivalent to its partner, but doesn't
translate the fmt message parameter.
errmsg
errmsg_internal - same as errmsg, but doesn't translate the fmt string
errdetail
errdetail_internal - same but errdetail, doesn't translate the fmt string
errhint
errhint_internal - no such thing ???
I noticed today that there is no 'errhint_internal' function partner
for the 'errhint' function.
Now, it might seem that hints are always intended for user output so
of course, you'll always want them translated.... but there are some
calls to this function (like below) where the actual hint message is
already built and translated before %s parameter substitution, so
AFAICT translation aka gettext lookup of just a "%s" format string
doesn't really achieve anything.
$ grep -r . -e 'errhint("%s"' | grep .c:
./contrib/dblink/dblink.c: message_hint ? errhint("%s", message_hint) : 0,
./contrib/postgres_fdw/connection.c: message_hint ? errhint("%s",
message_hint) : 0,
./src/backend/commands/vacuum.c: hintmsg ? errhint("%s", _(hintmsg)) : 0));
./src/backend/commands/tablecmds.c: (wentry->kind != '\0') ?
errhint("%s", _(wentry->drophint_msg)) : 0));
./src/backend/commands/tablecmds.c: errhint("%s", _(view_updatable_error))));
./src/backend/commands/view.c: errhint("%s", _(view_updatable_error))));
./src/backend/utils/misc/guc.c: hintmsg ? errhint("%s", _(hintmsg)) : 0));
./src/backend/utils/misc/guc.c: hintmsg ? errhint("%s", _(hintmsg)) : 0));
./src/backend/utils/misc/guc.c: hintmsg ? errhint("%s", _(hintmsg)) : 0));
./src/backend/utils/misc/guc.c: errhint("%s", GUC_check_errhint_string) : 0));
./src/backend/utils/misc/guc.c: errhint("%s", GUC_check_errhint_string) : 0));
./src/backend/utils/misc/guc.c: errhint("%s", GUC_check_errhint_string) : 0));
./src/backend/utils/misc/guc.c: errhint("%s", GUC_check_errhint_string) : 0));
./src/backend/utils/misc/guc.c: errhint("%s", GUC_check_errhint_string) : 0));
./src/pl/plpgsql/src/pl_exec.c: (err_hint != NULL) ? errhint("%s",
err_hint) : 0,
./src/pl/plpython/plpy_elog.c: (hint) ? errhint("%s", hint) : 0,
./src/pl/plpython/plpy_plpymodule.c: (hint != NULL) ? errhint("%s", hint) : 0,
./tmp.txt:src/backend/utils/misc/guc.c: errhint("%s",
GUC_check_errhint_string) : 0));
./tmp.txt:src/backend/utils/misc/guc.c: errhint("%s",
GUC_check_errhint_string) : 0));
./tmp.txt:src/backend/utils/misc/guc.c: errhint("%s",
GUC_check_errhint_string) : 0));
./tmp.txt:src/backend/utils/misc/guc.c: errhint("%s",
GUC_check_errhint_string) : 0));
./tmp.txt:src/backend/utils/misc/guc.c: errhint("%s",
GUC_check_errhint_string) : 0));
~
I wondered if such code as in those examples might prefer to call
errhint_internal to avoid making an unnecessary gettext lookup of
"%s".
OTOH, was an errhint_internal function deliberately omitted because
calling a superfluous gettext was not considered important enough to
bother?
======
Also, quite similar to this question --- I found a bunch of errmsg and
errdetail calls where the fmt string is just "%s". Are those correct,
or should those really be using the XXX_internal version of the
function instead?
$ grep -r . -e 'errmsg("%s"' | grep .c:
./contrib/pgcrypto/px.c: errmsg("%s", px_strerror(err))));
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV))),
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV)))));
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV)))));
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV)))));
./src/pl/plperl/plperl.c: errmsg("%s", strip_trailing_ws(sv2cstr(ERRSV)))));
./src/pl/tcl/pltcl.c: errmsg("%s", emsg),
./src/pl/tcl/pltcl.c: errmsg("%s", UTF_U2E(Tcl_GetString(objv[2])))));
$ grep -r . -e '\serrdetail("%s"' | grep .c:
./src/backend/executor/execExprInterp.c: errdetail("%s",
jsestate->escontext.error_data->message)));
./src/backend/executor/execExprInterp.c: errdetail("%s",
jsestate->escontext.error_data->message)));
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
I noticed today that there is no 'errhint_internal' function partner
for the 'errhint' function.
Now, it might seem that hints are always intended for user output so
of course, you'll always want them translated...
Yeah, I think that was the reasoning. If it needs a hint it must be
a user-facing error.
but there are some
calls to this function (like below) where the actual hint message is
already built and translated before %s parameter substitution, so
AFAICT translation aka gettext lookup of just a "%s" format string
doesn't really achieve anything.
There are lots of places where we don't worry about that overhead.
I'm not excited about trying to get rid of it everywhere --- surely
these paths are not very performance-critical?
regards, tom lane
(I added Andres to this thread)
Hi Andres,
I saw that a new errhint_internal() function was recently committed
[1]: https://github.com/postgres/postgres/commit/4244cf68769773ba30b868354f1f2fe93238e98b
month ago [2]/messages/by-id/CAHut+PtDHRif49G+bzspOGspETym5oKseD13v0tcBJXWUrTx9A@mail.gmail.com.
But, your patch only added the new function -- it does not make any
use of it for existing code that was using the errhint("%s", str)
method.
I wondered, given your commit message "but that's not exactly pretty
and makes it harder to avoid memory leaks", if you think it is
worthwhile to revisit those existing "%s" usages and modify them to
use the new errhint_internal? Tom above [3]/messages/by-id/547688.1738630459@sss.pgh.pa.us seemed not keen to modify
those without performance reasons, although at that time
errhint_internal didn't even exist.
======
[1]: https://github.com/postgres/postgres/commit/4244cf68769773ba30b868354f1f2fe93238e98b
[2]: /messages/by-id/CAHut+PtDHRif49G+bzspOGspETym5oKseD13v0tcBJXWUrTx9A@mail.gmail.com
[3]: /messages/by-id/547688.1738630459@sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi,
On 2025-04-03 09:58:30 +1100, Peter Smith wrote:
I saw that a new errhint_internal() function was recently committed
[1]. I had also posted above asking about this same missing function a
month ago [2].But, your patch only added the new function -- it does not make any
use of it for existing code that was using the errhint("%s", str)
method.I wondered, given your commit message "but that's not exactly pretty
and makes it harder to avoid memory leaks", if you think it is
worthwhile to revisit those existing "%s" usages and modify them to
use the new errhint_internal? Tom above [3] seemed not keen to modify
those without performance reasons, although at that time
errhint_internal didn't even exist.
I'd not go around and just categorically convert all users of errhint("%s",
str), that's probably not worth the noise. And plenty of them won't
benefit. E.g. the first one I just looked at is dblink_res_error(), where I
don't think using it would bring meaningful benefit. I suspect a bunch of
other places are going to be similar.
But I could also imageine there are some places where it'd improve the code /
remove unnecessary allocations.
Greetings,
Andres Freund
On Thu, Apr 3, 2025 at 10:26 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2025-04-03 09:58:30 +1100, Peter Smith wrote:
I saw that a new errhint_internal() function was recently committed
[1]. I had also posted above asking about this same missing function a
month ago [2].But, your patch only added the new function -- it does not make any
use of it for existing code that was using the errhint("%s", str)
method.I wondered, given your commit message "but that's not exactly pretty
and makes it harder to avoid memory leaks", if you think it is
worthwhile to revisit those existing "%s" usages and modify them to
use the new errhint_internal? Tom above [3] seemed not keen to modify
those without performance reasons, although at that time
errhint_internal didn't even exist.I'd not go around and just categorically convert all users of errhint("%s",
str), that's probably not worth the noise. And plenty of them won't
benefit. E.g. the first one I just looked at is dblink_res_error(), where I
don't think using it would bring meaningful benefit. I suspect a bunch of
other places are going to be similar.
Yes, I found 19 examples, but they are all similar to that.
I think they could all be changed
FROM
errhint("%s", str)
TO one of
errhint_internal("%s", str)
errhint_internal(str)
...but due to noise/benefit trade-off, I won't bother.
Thanks for your feedback.
======
Kind Regards,
Peter Smith.
Fujitsu Australia