Обсуждение: refcnt leak ?

Hi

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;/* * This next step is critical to prevent EndPlan() from trying to
close
 * an already-closed-and-deleted RelationInfo ---
es_result_relation_info * is pointing at one of the nodes we just zapped above. */estate->es_result_relation_info =
NULL;

This seems to cause a refcnt leak when
appendstate->as_result_relation_info_list is NIL from the first
e.g. in the case INSERT INTO .. SELECT ...

Comments ?

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

Regards.
Hiroshi Inoue

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