Re: Forget close an open relation in ReorderBufferProcessTXN()

On Thu, May 13, 2021 at 7:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Apr 20, 2021 at 8:36 AM 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%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.
> >
>
> It seems in get_rel_sync_entry, it will only build the map again when
> there is any invalidation in publication_rel. Don't we need to build
> it after any DDL on the relation itself? I haven't tried this with a
> test so I might be missing something.

That's a good point, I didn't really think that through. So,
rel_sync_cache_relation_cb(), that gets called when the published
table's relcache is invalidated, only resets schema_sent but not
replicate_valid. The latter, as you said, is reset by
rel_sync_cache_publication_cb() when a pg_publication syscache
invalidation occurs. So with the patch, it's possible for the map to
not be recreated, even when it should, if for example DDL changes the
table's TupleDesc.

I have put the map-creation code back into maybe_send_schema() in the
attached updated patch, updated some comments related to the map, and
added a test case that would fail with the previous patch (due to
moving map-creation code into get_rel_sync_entry() that is) but
succeeds with the updated patch.

> Also, don't we need to free the
> entire map as suggested by me?

Yes, I had missed that too. Addressed in the updated patch.

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