On Tue, Apr 6, 2021 at 1:57 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Tue, Apr 6, 2021 at 1:15 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Tue, Apr 6, 2021 at 1:01 PM houzj.f...@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
fix_relcache_leak_in_lrworker_v4.patch
Description: Binary data
