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
