On Tuesday, April 20, 2021 12:07 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Sat, Apr 17, 2021 at 1:30 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > On Fri, Apr 16, 2021 at 11:24 PM Andres Freund <and...@anarazel.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