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 > <houzj.f...@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/
