From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Amit Langote <amitlangote09(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Japin Li <japinli(at)hotmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Forget close an open relation in ReorderBufferProcessTXN() |
Date: | 2021-05-13 10:43:27 |
Message-ID: | CAA4eK1+R_+KfFwY=LtByYjEo=JV2AoNWXLtGkcGp0SVcs3FuqQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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. Also, don't we need to free the
entire map as suggested by me?
--
With Regards,
Amit Kapila.
From | Date | Subject | |
---|---|---|---|
Next Message | vignesh C | 2021-05-13 10:44:55 | Re: subscriptioncheck failure |
Previous Message | Amit Kapila | 2021-05-13 10:21:13 | Re: Forget close an open relation in ReorderBufferProcessTXN() |