Thread: Re: Question -- why is there no errhint_internal function?

(I added Andres to this thread)

Hi Andres,

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.

======
[1] https://github.com/postgres/postgres/commit/4244cf68769773ba30b868354f1f2fe93238e98b
[2] https://www.postgresql.org/message-id/CAHut%2BPtDHRif49G%2BbzspOGspETym5oKseD13v0tcBJXWUrTx9A%40mail.gmail.com
[3] https://www.postgresql.org/message-id/547688.1738630459%40sss.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