Re: Forget close an open relation in ReorderBufferProcessTXN()

On Sat, Apr 17, 2021 at 1:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Apr 16, 2021 at 11:24 PM Andres Freund <andres(at)anarazel(dot)de> wrote:> > This made me take a brief look at pgoutput.c - maybe I am missing
> > something, but how is the following not a memory leak?
> >
> > static void
> > maybe_send_schema(LogicalDecodingContext *ctx,
> > ReorderBufferTXN *txn, ReorderBufferChange *change,
> > Relation relation, RelationSyncEntry *relentry)
> > {
> > ...
> > /* Map must live as long as the session does. */
> > oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> > relentry->map = convert_tuples_by_name(CreateTupleDescCopy(indesc),
> > CreateTupleDescCopy(outdesc));
> > MemoryContextSwitchTo(oldctx);
> > send_relation_and_attrs(ancestor, xid, ctx);
> > RelationClose(ancestor);
> >
> > If - and that's common - convert_tuples_by_name() won't have to do
> > anything, the copied tuple descs will be permanently leaked.
> >
>
> I also think this is a permanent leak. I think we need to free all the
> memory associated with this map on the invalidation of this particular
> relsync entry (basically in rel_sync_cache_relation_cb).

I agree there's a problem here.

Back in:

https://www.postgresql.org/message-id/CA%2BHiwqEeU19iQgjN6HF1HTPU0L5%2BJxyS5CmxaOVGNXBAfUY06Q%40mail.gmail.com

I had proposed to move the map creation from maybe_send_schema() to
get_rel_sync_entry(), mainly because the latter is where I realized it
belongs, though a bit too late. Attached is the part of the patch
for this particular issue. It also takes care to release the copied
TupleDescs if the map is found to be unnecessary, thus preventing
leaking into CacheMemoryContext.

--
Amit Langote
EDB: http://www.enterprisedb.com