On Tue, Apr 20, 2021 at 8:36 AM 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%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.
