On Fri, 2026-03-13 at 09:54 -0400, Andres Freund wrote:
> GetSubscription() does a dozen allocations or so, so I'm not sure it
> matters
> that ForeignServerConnectionString() would do another few. There is
> FreeSubscription(), but it only seems to free a subset of the
> allocations, and
> none of the temporary allocations that are surely made below
> GetSubscription().
That by itself seems like a problem. If FreeSubscription() is needed,
then it should do its job; and if it's not needed, then it should be
eliminated.
To me it looks like it's needed. Otherwise there will be leaks for
every invalidation.
> But regardless of that, even if you want to make
> ForeignServerConnectionString() not leak, I don't think that means
> you need to
> use PG_TRY/FINALLY. If there's an error, afaict the callers will just
> throw
> away the apply context, no? Otherwise there'd likely be way more
> problems.
I believe that's correct. While there's no delete or reset, it looks
like an error will cause the worker to exit/restart.
> So
> just create the context without a PG_TRY and delete it in the success
> case.
> In the failure case it'll be cleaned up by error handling.
Thank you, patch attached.
> And if you do need the PG_TRY for some reason, why not do the
> text_to_cstring() call inside the PG_TRY, since it never can have a
> value if
> an error was thrown?
That's what I did in the patch I posted here:
https://www.postgresql.org/message-id/f48610c90e69de4b30841361c568c3765e8f3dfe.camel%40j-davis.com
but I think you are right that we don't need the try/catch at all.
Regards,
Jeff Davis
