pgsql: Remove inappropriate memory context switch in

On Sun, Nov 30, 2008 at 10:49 AM, Tom Lane <tgl@postgresql.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@samurai.com> writes:

On Sun, Nov 30, 2008 at 10:49 AM, Tom Lane <tgl@postgresql.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@sss.pgh.pa.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@samurai.com> writes:

On Sun, Nov 30, 2008 at 1:09 PM, Tom Lane <tgl@sss.pgh.pa.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@samurai.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