refcnt leak ?

Hiroshi Inoue <Inoue@tpf.co.jp> writes:

While examining recursive use of catalog cache,I found
a refcnt leak of relations.
After further investigation,I found that the following seems
to be the cause.

[ in EndAppend() in nodeAppend.c ]

appendstate-> as_result_relation_info_list = NIL;

That doesn't look like a problem to me --- the result relations *have*
been closed, just above this line.

BTW,doesn't EndAppend() neglect to call ExecCloseIndices()
for RelationInfos of appendstate->as_result_relation_info_list ?

Comparing nodeAppend to EndPlan(), I think you are right --- each
resultinfo should have ExecCloseIndices applied too, in the loop just
above the line you quote. This did not use to be a problem because
Append plans were readonly, but now that we have UPDATE/DELETE on
inheritance hierarchies, there's a missing step here. Was your test
query of that kind?

regards, tom lane

On Tue, 7 Nov 2000, Tom Lane wrote:

Hiroshi Inoue <Inoue@tpf.co.jp> writes:

While examining recursive use of catalog cache,I found
a refcnt leak of relations.
After further investigation,I found that the following seems
to be the cause.

[ in EndAppend() in nodeAppend.c ]

appendstate-> as_result_relation_info_list = NIL;

That doesn't look like a problem to me --- the result relations *have*
been closed, just above this line.

BTW,doesn't EndAppend() neglect to call ExecCloseIndices()
for RelationInfos of appendstate->as_result_relation_info_list ?

Comparing nodeAppend to EndPlan(), I think you are right --- each
resultinfo should have ExecCloseIndices applied too, in the loop just
above the line you quote. This did not use to be a problem because
Append plans were readonly, but now that we have UPDATE/DELETE on
inheritance hierarchies, there's a missing step here. Was your test
query of that kind?

Show anything configure's switch --enable-cassert? IMHO real leak must
be *probably* visible with this compile option in 7.1 (I hope:-).

Karel

-----Original Message-----
From: Tom Lane [mailto:tgl@sss.pgh.pa.us]

Hiroshi Inoue <Inoue@tpf.co.jp> writes:

While examining recursive use of catalog cache,I found
a refcnt leak of relations.
After further investigation,I found that the following seems
to be the cause.

[ in EndAppend() in nodeAppend.c ]

appendstate-> as_result_relation_info_list = NIL;

Oh no,my point isn't on this line but on the line

estate->es_result_relation_info = NULL;

As the comment says,it depends on the assumption that
estate->es_result_relation_info points to one of the node
of appendstate->as_result_relation_info_list(before set to
NIL). However ISTM appendstate->as_result_relation_info
_list is for inheritance and in the case "INSERT INTO ..
SELECT .. FROM .." it's not used.

That doesn't look like a problem to me --- the result relations *have*
been closed, just above this line.

BTW,doesn't EndAppend() neglect to call ExecCloseIndices()
for RelationInfos of appendstate->as_result_relation_info_list ?

Comparing nodeAppend to EndPlan(), I think you are right --- each
resultinfo should have ExecCloseIndices applied too, in the loop just
above the line you quote. This did not use to be a problem because
Append plans were readonly, but now that we have UPDATE/DELETE on
inheritance hierarchies, there's a missing step here. Was your test
query of that kind?

I first changed this part but rd_refcnt leak didn't disappaear.
I have no refcnt leak example which is caused due to this flaw(?).
After that I found " estate->es_result_relation_info = NULL; "
in EndAppend() . I changed it to not do so when appendstate->
as_result_relation_info_list is NIL and rd_refcnt leak disappeared.

Regards.
Hiroshi Inoue

"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:

Oh no,my point isn't on this line but on the line
estate->es_result_relation_info = NULL;

Oh, I see --- this is mistakenly assuming that es_result_relation_info
*always* points at one of the Append's relations. So there are actually
two rel-refcnt-leaking bugs here, this one and the lack of index close.

I've fixed both. Thanks for the report!

regards, tom lane