Re: Memory leak in WAL sender with pgoutput (v10~)

On Tue, Dec 3, 2024 at 2:12 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2024-Dec-02, Amit Kapila wrote:
>
> > 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.
>
> I don't necessarily agree, given that this context is not needed
> anywhere else.
>

But that suits the current design more. We allocate PGOutputData and
other contexts in that structure in a "Logical decoding context". A
few of its members (publications, publication_names) residing in
totally unrelated contexts sounds odd. In the first place, we don't
need to allocate publications under CacheMemoryContext, they should be
allocated in PGOutputData->cachectx. However, because we need to free
those entirely at one-shot during invalidation processing, we could
use a new context as a child context of PGOutputData->cachectx. Unless
I am missing something, the current memory context usage appears more
like a coding convenience than a thoughtful design decision.

--
With Regards,
Amit Kapila.