Re: Table refer leak in logical replication

On Tue, Apr 6, 2021 at 1:15 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Tue, Apr 6, 2021 at 1:01 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.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.

Yeah, I had missed it. Thank you for pointing out it.

>
> 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.

Looks good to me.

BTW I found the following comments in create_estate_for_relation():

/*
 * Executor state preparation for evaluation of constraint expressions,
 * indexes and triggers.
 *
 * This is based on similar code in copy.c
 */
static EState *
create_estate_for_relation(LogicalRepRelMapEntry *rel)

It seems like the comments meant the code around CopyFrom() and
DoCopy() but it would no longer be true since copy.c has been split
into some files and I don't find similar code in copy.c. I think it’s
better to remove the sentence rather than update the file name as this
comment doesn’t really informative and hard to track the updates. What
do you think?

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/