Re: Table refer leak in logical replication

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.

--
With Regards,
Amit Kapila.