On Tue, Dec 3, 2024 at 11:57 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
>
The parent structure (PGOutputData) was stored in the "Logical
decoding context" even in v11. So, how does storing its member
'publications' in CacheMemoryContext a good idea? It is possible that
we are leaking memory while doing decoding via SQL APIs where we free
decoding context after getting changes though I haven't tested the
same.
--
With Regards,
Amit Kapila.
