Re: Table refer leak in logical replication

I added this as an Open Item.
https://wiki.postgresql.org/index.php?title=PostgreSQL_14_Open_Items&type=revision&diff=35895&oldid=35890
https://www.postgresql.org/message-id/flat/OS0PR01MB6113BA0A760C9964A4A0C5C2FB769%40OS0PR01MB6113.jpnprd01.prod.outlook.com#2fc410dff5cd27eea357ffc17fc174f2

On Tue, Apr 06, 2021 at 02:25:05PM +0900, Amit Langote wrote:
> On Tue, Apr 6, 2021 at 1:57 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Tue, Apr 6, 2021 at 1:15 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > On Tue, Apr 6, 2021 at 1:01 PM houzj(dot)fnst(at)fujitsu(dot)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.