Thread: 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@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