Re: Table refer leak in logical replication

On Tue, Apr 6, 2021 at 1:01 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > > insert into test values (6);
> > > >
> > > > It seems an issue about reference leak. Anyone can fix this?
> > >
> > > It seems ExecGetTriggerResultRel will reopen the target table because it
> > cannot find an existing one.
> > > Storing the opened table in estate->es_opened_result_relations seems
> > solves the problem.
> >
> > It seems like commit 1375422c is related to this bug.

Right, thanks for pointing this out.

> The commit introduced a
> > new function ExecInitResultRelation() that sets both
> > estate->es_result_relations and estate->es_opened_result_relations. I
> > think it's better to use ExecInitResultRelation() rather than directly setting
> > estate->es_opened_result_relations. It might be better to do that in
> > create_estate_for_relation() though. Please find an attached patch.

Agree that ExecInitResultRelations() would be better.

> > Since this issue happens on only HEAD and it seems an oversight of commit
> > 1375422c, I don't think regression tests for this are essential.
>
> It seems we can not only use ExecInitResultRelation.
> In function ExecInitResultRelation, it will use ExecGetRangeTableRelation which
> will also open the target table and store the rel in "Estate->es_relations".
> We should call ExecCloseRangeTableRelations at the end of apply_handle_xxx to
> close the rel in "Estate->es_relations".

Right, ExecCloseRangeTableRelations() was missing.

I think it may be better to create a sibling function to
create_estate_for_relation(), say, close_estate(EState *), that
performs the cleanup actions, including the firing of any AFTER
triggers. See attached updated patch to see what I mean.

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