Re: Table refer leak in logical replication

On Tue, Apr 6, 2021 at 1:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 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
> > > 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?

Yeah, agree with simply removing that comment.

While updating the patch to do so, it occurred to me that maybe we
could move the ExecInitResultRelation() call into
create_estate_for_relation() too, in the spirit of removing
duplicative code.  See attached updated patch.  Actually I remember
proposing that as part of the commit you shared in your earlier email,
but for some reason it didn't end up in the commit.  I now think maybe
we should do that after all.

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