Re: Forget close an open relation in ReorderBufferProcessTXN()

Takamichi-san,

On Tue, Apr 27, 2021 at 9:37 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> 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.

Thanks for checking.

> 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 ?

I have not used valgrind or other testing methods to check this.

To me, it's clear in this case by only looking at the code that the
TupleDescs returned by CreateTupleDescCopy() ought to be freed when
convert_tuples_by_name() determines that no map is necessary such that
there will be no need to keep those TupleDesc copies around. Failing
that, those copies end up in CacheMemoryContext without anything
pointing to them, hence the leak. Actually, since maybe_send_schema()
doesn't execute this code if schema_sent is found to have been set by
earlier calls, the leak in question should occur only once in most
tests.

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