Thread: SRF memory leaks

Attached is a patch that fixes the SRF memory leaks I recently reported
on -hackers[1]. The patch creates a distinct memory context for the
SRF's "multi_call_memory_ctx", and then deletes that context when the
SRF finishes. This ensures that any user allocations made in this
context are reclaimed. The patch also frees a TupleDesc that was leaked
in the per-query context when nodeFunctionscan was rescanned. Along the
way, it also fixes a leak in shutdown_MultiFuncCall() ("attinmeta" was
freed, but its palloc'd fields were not.)

It would be possible to allocate the TupleDesc in the multi-call memory
context, but it doesn't seem worth bothering about to me (since it would
require passing the context from the RSI down to the FuncCallContext). I
also didn't try to have multiple SRFs in the same subquery block use the
same context -- that seems like a lot of pain for little gain.

Comments welcome -- I think this fix should be applied to back branches.

-Neil

[1] http://markmail.org/message/45hekjinzl3e5i6q

Neil Conway <neilc@samurai.com> writes:
>           if (funcTupdesc)
> +         {
>               tupledesc_match(node->tupdesc, funcTupdesc);
> +             FreeTupleDesc(funcTupdesc);
> +         }
>       }

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.  We certainly daren't
backpatch such a change.

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.

            regards, tom lane

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

Neil Conway <neilc@samurai.com> writes:
> 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.

> AFAICS this is not true of any of the SRFs in the backend, which always
> return expendable tupdescs.

"It's OK in the built-in SRFs" is disastrously different from "It's OK".

It was never specified that SRFs had to return a free-able tupdesc,
so I think it's a lead pipe cinch that there are some out there that
don't.  Nor would it be their fault if we change the specification.

            regards, tom lane

On Tue, 2008-02-26 at 03:13 -0500, Tom Lane wrote:
> "It's OK in the built-in SRFs" is disastrously different from "It's OK".

Right, I never said that, I was just commenting on your view that the
predominant use-case for SRFs is returning refcounted tupdescs.

You didn't comment on my proposed solution (FreeTupleDesc() iff refcount
== -1). ISTM that we *need* to free the TupleDesc in at least some
cases, in order to defend against the practice of explicitly allocating
the TupleDesc in the per-query context.

-Neil

On Tue, 2008-02-26 at 00:17 -0800, Neil Conway wrote:
> You didn't comment on my proposed solution (FreeTupleDesc() iff refcount
> == -1).

Attached is a revised version of this patch. It makes the
FreeTupleDesc() change described above, and fixes a bug: in
SRF_RETURN_DONE(), we need to be sure to switch back to the caller's
context before deleting the multi_call_ctx, since some SRFs (e.g.
dblink) call SRF_RETURN_DONE() while still inside the multi_call_ctx.

I'd like to apply this change to back branches reasonably soon, so if
you have a better way to do the FreeTupleDesc() hack, let me know.

-Neil

On Tue, 2008-02-26 at 12:09 -0800, Neil Conway wrote:
> I'd like to apply this change to back branches reasonably soon, so if
> you have a better way to do the FreeTupleDesc() hack, let me know.

Barring any objections, I'll apply this to HEAD and back branches
tonight or tomorrow.

-Neil

Neil Conway <neilc@samurai.com> writes:
> On Tue, 2008-02-26 at 00:17 -0800, Neil Conway wrote:
>> You didn't comment on my proposed solution (FreeTupleDesc() iff refcount
>> == -1).

I still find that entirely unsafe, particularly for something you
propose to back-patch into stable branches.  Negative refcount does
not prove that the SRF itself hasn't still got a pointer to the tupdesc.

Can't we fix it so that the tupdesc is allocated in the new special
context (at least in the primary code paths), and then no explicit
free is needed?

            regards, tom lane

On Wed, 2008-02-27 at 15:07 -0500, Tom Lane wrote:
> Negative refcount does not prove that the SRF itself hasn't
> still got a pointer to the tupdesc.

That sounds quite bizarre. The SRF has already finished execution at
this point, so keeping a pointer to the tupledesc around would only make
sense if you wanted to use that tupledesc on a *subsequent* invocation
of the SRF. The SRF would need to store the pointer in a static
variable, too, and it wouldn't have an easy way to distinguish between
repeated calls to the SRF within the same query and in different queries
(since in the latter case the TupleDesc will be long gone anyway). I
can't see why anyone would want to do this.

> Can't we fix it so that the tupdesc is allocated in the new special
> context (at least in the primary code paths), and then no explicit
> free is needed?

As I said earlier, the tupdesc is explicitly allocated in the per-query
context by the SRFs themselves. If by "primary code paths", you mean
"SRFs in the main source tree", then sure, we can make arbitrary changes
to those. That won't help out-of-tree SRFs though.

-Neil

Neil Conway <neilc@samurai.com> writes:
> On Wed, 2008-02-27 at 15:07 -0500, Tom Lane wrote:
>> Negative refcount does not prove that the SRF itself hasn't
>> still got a pointer to the tupdesc.

> That sounds quite bizarre. The SRF has already finished execution at
> this point, so keeping a pointer to the tupledesc around would only make
> sense if you wanted to use that tupledesc on a *subsequent* invocation
> of the SRF.

Hmm ... actually I was worried about it being embedded in the returned
tuplestore, but I see tuplestore doesn't currently use a tupdesc at all,
so maybe it isn't that big a problem.

            regards, tom lane

On Wed, 2008-02-27 at 10:54 -0800, Neil Conway wrote:
> Barring any objections, I'll apply this to HEAD and back branches
> tonight or tomorrow.

Applied to HEAD, REL8_3_STABLE, and REL8_2_STABLE. (I can backpatch
further if anyone feels strongly about it.)

-Neil