RE: Forget close an open relation in ReorderBufferProcessTXN()

On Tuesday, April 20, 2021 12:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> 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%2BHiwqEeU19iQgjN6HF1HTP
> U0L5%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.
Thank you for sharing the patch.
Your patch looks correct to me. Make check-world has
passed with it. Also, I agree with the idea to place
the processing to set the map in the get_rel_sync_entry.

One thing I'd like to ask is an advanced way to confirm
the memory leak is solved by the patch, not just by running make check-world.

I used OSS HEAD and valgrind, expecting that
I could see function stack which has a call of CreateTupleDescCopy
from both pgoutput_change and pgoutput_truncate as memory leak report
in the valgrind logs, and they disappear after applying the patch.

But, I cannot find the pair of pgoutput functions and CreateTupleDescCopy in one report
when I used OSS HEAD, which means that I need to do advanced testing to check if
the memory leak of CreateTupleDescCopy is addressed.
I collected the logs from RT at src/test/subscription so should pass the routes of our interest.

Could someone give me
an advise about the way to confirm the memory leak is solved ?

Best Regards,
Takamichi Osumi