pgsql: Remove inappropriate memory context switch in

Log Message:
-----------
Remove inappropriate memory context switch in shutdown_MultiFuncCall().
This was a thinko introduced in a patch from last February; it results
in memory leakage if an SRF is shut down before the actual end of query,
because subsequent code will be running in a longer-lived context than
it's expecting to be.

Modified Files:
--------------
pgsql/src/backend/utils/fmgr:
funcapi.c (r1.42 -> r1.43)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/fmgr/funcapi.c?r1=1.42&r2=1.43)

On Sun, Nov 30, 2008 at 10:49 AM, Tom Lane <tgl(at)postgresql(dot)org> wrote:
> Remove inappropriate memory context switch in shutdown_MultiFuncCall().
> This was a thinko introduced in a patch from last February; it results
> in memory leakage if an SRF is shut down before the actual end of query,
> because subsequent code will be running in a longer-lived context than
> it's expecting to be.

I added that context switch for a reason:

http://archives.postgresql.org/pgsql-patches/2008-02/msg00153.php

Presumably that's why the build farm is crashing on dblink.

Neil

"Neil Conway" <neilc(at)samurai(dot)com> writes:
> On Sun, Nov 30, 2008 at 10:49 AM, Tom Lane <tgl(at)postgresql(dot)org> wrote:
>> Remove inappropriate memory context switch in shutdown_MultiFuncCall().

> I added that context switch for a reason:
> http://archives.postgresql.org/pgsql-patches/2008-02/msg00153.php

Hm, too bad you didn't respond to my message inquiring about this a
couple days ago.

> Presumably that's why the build farm is crashing on dblink.

I'll take a look in a bit. However, the switch in the shutdown function
was simply wrong, because it was *not* "returning to the caller's
context". I suspect what this really says is dblink is doing something
it shouldn't.

regards, tom lane

On Sun, Nov 30, 2008 at 1:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hm, too bad you didn't respond to my message inquiring about this a
> couple days ago.

My apologies -- my mail filters were overeager, and I didn't see it.

> I'll take a look in a bit. However, the switch in the shutdown function
> was simply wrong, because it was *not* "returning to the caller's
> context". I suspect what this really says is dblink is doing something
> it shouldn't.

Well, dblink is simply calling SRF_RETURN_DONE() when the current
context is the multi-call memory context. We could outlaw that
practice, but that risks breaking out-of-tree SRFs that do something
similar. I think better would be to figure out a more appropriate
memory context to switch into before deleting the multi-call context.

Neil

"Neil Conway" <neilc(at)samurai(dot)com> writes:
> On Sun, Nov 30, 2008 at 1:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Well, dblink is simply calling SRF_RETURN_DONE() when the current
> context is the multi-call memory context.

So I see.

> We could outlaw that
> practice, but that risks breaking out-of-tree SRFs that do something
> similar.

No, they're already broken; this is just making the breakage more
obvious. It is not kosher for a SQL-callable function to return with
a different current context than it was called in.

regards, tom lane

"Neil Conway" <neilc(at)samurai(dot)com> writes:
> ... I think better would be to figure out a more appropriate
> memory context to switch into before deleting the multi-call context.

BTW, I did look at that alternative. In principle we could have
init_MultiFuncCall save the current context and let end_MultiFuncCall
switch to that context. However there are a couple of serious problems
with that:

1. There's noplace in FuncCallContext to save it. Adding a field would
represent an ABI break that is certain to break external modules, so
it seems a nonstarter for a back-patchable fix.

2. We can't be absolutely sure that this is the right context to return
to anyway --- perhaps the SRF changed the context before calling
init_MultiFuncCall. (One plausible mechanism for this is if SPI_connect
is called first.) SRF_RETURN_DONE() doesn't give the caller any chance
to fix things afterwards, so a 95% or 99% solution isn't good enough.

In general, I think funcapi.c has no business making a change to the
caller's CurrentMemoryContext in any case. It never did so before the
February patch, and is not documented to do so.

regards, tom lane