Horton

Thread

  1. pgsql: Remove inappropriate memory context switch in

    Tom Lane <tgl@postgresql.org> — 2008-11-30T18:49:36Z

    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)

  2. Re: pgsql: Remove inappropriate memory context switch in

    Neil Conway <neilc@samurai.com> — 2008-11-30T21:03:29Z

    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

  3. Re: pgsql: Remove inappropriate memory context switch in

    Tom Lane <tgl@sss.pgh.pa.us> — 2008-11-30T21:09:18Z

    "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

  4. Re: pgsql: Remove inappropriate memory context switch in

    Neil Conway <neilc@samurai.com> — 2008-11-30T21:26:17Z

    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

  5. Re: pgsql: Remove inappropriate memory context switch in

    Tom Lane <tgl@sss.pgh.pa.us> — 2008-11-30T21:29:03Z

    "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

  6. Re: pgsql: Remove inappropriate memory context switch in

    Tom Lane <tgl@sss.pgh.pa.us> — 2008-11-30T23:07:09Z

    "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