Hi,
On 2026-03-13 13:37:29 -0700, Jeff Davis wrote:
> 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.
No argument there.
> To me it looks like it's needed. Otherwise there will be leaks for
> every invalidation.
It does indeed look like it'd leak.
I suspect the more scalable approach would be to create a dedicated memory
context for each GetSubscription() call that's then torn down during
invalidation.
> > 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.
LGTM on a quick scan.
> > 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.
I was behind on my email, so I hadn't yet seen that :)
Greetings,
Andres
