On Thu, May 13, 2021 at 7:43 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> 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.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
pgoutput-tupeconv-map-leak_v2.patch
Description: Binary data
