On Mon, 2008-02-25 at 21:00 -0500, Tom Lane wrote:
> I find this part of the patch to be a seriously bad idea.
> nodeFunctionscan has no right to assume that the function has returned
> an expendable tupdesc; indeed, I would think that the other case is
> more nearly what's expected by the API for SRFs.
AFAICS this is not true of any of the SRFs in the backend, which always
return expendable tupdescs.
> A safer fix would be to try to make the tupdesc be in the new
> multi_call_ctx when it's being created by the funcapi.c code.
funcapi.c doesn't create the tupdesc, though: it is created by user
code, and merely assigned to a field of the RSI (note that the TupleDesc
in question is the one in the ReturnSetInfo, not in FuncCallContext.) A
minor detail is that the lifetime of the tupdesc also needs to exceed
the lifetime of the multi_call_ctx, at least as currently implemented.
From looking at the existing SRFs in the backend, the TupleDesc is
always *explicitly* allocated in the estate's per-query context, so it
will leak unless freed. Perhaps it would be sufficient to FreeTupleDesc
iff tupdesc->refcount == -1?
BTW, I'm thinking of changing the various SRFs that make allocations in
the EState's per-query context to instead use the SRF's multi_call_ctx.
This means the memory will be reclaimed sooner and avoids possible
memory leaks.
-Neil