Re: Table refer leak in logical replication

On Mon, Apr 19, 2021 at 7:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Apr 19, 2021 at 3:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Mon, Apr 19, 2021 at 3:12 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > On Mon, Apr 19, 2021 at 3:02 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > > > On Mon, Apr 19, 2021 at 02:33:10PM +0530, Amit Kapila wrote:
> > > > > On Mon, Apr 19, 2021 at 12:32 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > > >> FWIW, I agree with fixing this bug of 1375422c in as least scary
> > > > >> manner as possible. Hou-san proposed that we add the ResultRelInfo
> > > > >> that apply_handle_{insert|update|delete} initialize themselves to
> > > > >> es_opened_result_relations. I would prefer that only
> > > > >> ExecInitResultRelation() add anything to es_opened_result_relations()
> > > > >> to avoid future maintenance problems. Instead, a fix as simple as the
> > > > >> Hou-san's proposed fix would be to add a ExecCloseResultRelations()
> > > > >> call at the end of each of apply_handle_{insert|update|delete}.
> > > > >
> > > > > Yeah, that will work too but might look a bit strange. BTW, how that
> > > > > is taken care of for ExecuteTruncateGuts? I mean we do add rels there
> > > > > like Hou-San's patch without calling ExecCloseResultRelations, the
> > > > > rels are probably closed when we close the relation in worker.c but
> > > > > what about memory for the list?
> > > >
> > > > TRUNCATE relies on FreeExecutorState() for that, no?
> > > >
> > >
> > > I am not sure about that because it doesn't seem to be allocated in
> > > es_query_cxt. Note, we switch to oldcontext in the
> > > CreateExecutorState.
> > >
> >
> > I have just checked that the memory for the list is allocated in
> > ApplyMessageContext. So, it appears a memory leak to me unless I am
> > missing something.
> >
>
> It seems like the memory will be freed after we apply the truncate
> because we reset the ApplyMessageContext after applying each message,
> so maybe we don't need to bother about it.

Yes, ApplyMessageContext seems short-lived enough for this not to
matter. In the case of ExecuteTruncateGuts(), it's PortalContext, but
we don't seem to bother about leaking into that one too.

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