On Mon, Dec 2, 2024 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Nov 30, 2024 at 09:28:31AM +0100, Alvaro Herrera wrote:
> > I'm not sure about your proposed fix. Isn't it simpler to have another
> > memory context which we can reset instead of doing list_free_deep()? It
> > doesn't have to be a global memory context -- since this is not
> > reentrant and not referenced anywhere else, it can be a simple static
> > variable in that block, as in the attached. I ran the stock tests (no
> > sysbench) and at least it doesn't crash.
> >
> > This should be easily backpatchable also, since there's no ABI change.
>
> Yeah. Using more memory contexts around these API calls done without
> the cache manipulation is something I've also mentioned to Amit a few
> days ago, and I'm feeling that this concept should be applied to a
> broader area than just the cached publication list in light of this
> thread and ea792bfd93ab.
>
We already have PGOutputData->cachectx which could be used for it. I
think we should be able to reset such a context when we are
revalidating the publications. Even, if we want a new context for some
localized handling, we should add that in PGOutputData rather than a
local context as the proposed patch is doing at the very least for
HEAD.
Can't we consider freeing the publication names individually that can
be backpatchable and have no or minimal risk of breaking anything?
>
> I am slightly concerned about the current design of GetPublication()
> in the long-term, TBH. LoadPublications() has hidden the leak behind
> two layers of routines in the WAL sender, and that's easy to miss once
> you call anything that loads a Publication depending on how the caller
> caches its data. So I would still choose for modifying the structure
> on HEAD removing the pstrdup() for the publication name.
>
BTW, the subscription structure also used the name in a similar way.
This will make the publication/subscription names handled differently.
--
With Regards,
Amit Kapila.
