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

On Tue, Dec 03, 2024 at 09:50:42AM +0530, Amit Kapila wrote:
> 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.

PGOutputData->cachectx has been introduced in 2022 in commit 52e4f0cd4,
while the decision to have RelationSyncEntry and the publication list
in CacheMemoryContext gets down to v10 where this logical replication
has been introduced. This was a carefully-thought choice back then
because this is data that belongs to the process cache, so yes, this
choice makes sense to me. Even today this choice makes sense: this is
still data cached in the process, and it's stored in the memory
context for cached data.

The problem is that we have two locations where the cached data is
stored, and I'd agree to make the whole leaner by relying on one or
the other entirely, but not both. If you want to move all that to the
single memory context in PGOutputData, perhaps that makes sense in the
long-run and the code gets simpler. Perhaps also it could be simpler
to get everything under CacheMemoryContext and remove
PGOutputData->cachectx. However, if you do so, I'd suggest to use the
same parent context for RelationSyncData as well rather than have two
concepts, not only the publication list.

That's digressing from the original subject a bit, btw..
--
Michael