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. 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. -- Amit Langote EDB: http://www.enterprisedb.com
fix_relcache_leak_in_lrworker_v3.patch
Description: Binary data
