Horton
Menu

Thread

  1. Tuplestore should remember the memory context it's created in

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-22T11:45:50Z

    With regards to this bug report:
http://archives.postgresql.org/pgsql-bugs/2009-12/msg00241.php

I think we should change tuplestore code so that callers of
tuplestore_put* don't need to switch to the correct memory context (and
resource owner, after this patch) before call. Instead,
tuplestore_begin_heap() should memorize the context and resource owner
used to create the tuplestore, and use that in tuplestore_put*
functions. AFAICS it is always a bug to be in a different memory context
in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
robust to not put the burden on the callers.

Patch against CVS HEAD to do that and fix the reported bug attached. Now
that the tuplestore_put* switches to the right memory context, we could
remove that from all the callers, but this patch only does it for pl_exec.c.

Thoughts?

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  2. Re: Tuplestore should remember the memory context it's created in

    Greg Stark <gsstark@mit.edu> — 2009-12-22T12:16:38Z

    On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
...
> AFAICS it is always a bug to be in a different memory context
> in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
> robust to not put the burden on the callers.
> ...
> Patch against CVS HEAD to do that and fix the reported bug attached. Now
> that the tuplestore_put* switches to the right memory context, we could
> remove that from all the callers, but this patch only does it for pl_exec.c.
>
> Thoughts?

I thought there were comments specifically explaining why it was done
that way but I don't recall what they said. Perhaps it was a
performance concern since it's going to happen for every tuple put in
the tuplestore and usually you'll just be in the same memory context
anyways. It would certainly be a lot less confusing the way you
describe though.

-- 
greg

  3. Re: Tuplestore should remember the memory context it's created in

    Greg Stark <gsstark@mit.edu> — 2009-12-22T12:20:46Z

    On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> With regards to this bug report:
> http://archives.postgresql.org/pgsql-bugs/2009-12/msg00241.php

Hm, do we have any build farm members running with work_mem set to the minimum?

-- 
greg

  4. Re: Tuplestore should remember the memory context it's created in

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-22T15:01:44Z

    Greg Stark <gsstark@mit.edu> writes:
> On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> AFAICS it is always a bug to be in a different memory context
>> in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
>> robust to not put the burden on the callers.

> I thought there were comments specifically explaining why it was done
> that way but I don't recall what they said.

I think it was just a performance optimization.  It's probably not
measurable though; even in the in-memory case there's at least a palloc
inside the put() function, no?

			regards, tom lane

  5. Re: Tuplestore should remember the memory context it's created in

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-22T15:37:38Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Patch against CVS HEAD to do that and fix the reported bug attached. Now
> that the tuplestore_put* switches to the right memory context, we could
> remove that from all the callers, but this patch only does it for pl_exec.c.

BTW, I'm not convinced that the owner-switchery you added to pl_exec.c
is necessary/appropriate.  Under what circumstances would that be a good
idea?

			regards, tom lane

  6. Re: Tuplestore should remember the memory context it's created in

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-22T16:13:24Z

    Tom Lane wrote:
> Greg Stark <gsstark@mit.edu> writes:
>> On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
>> <heikki.linnakangas@enterprisedb.com> wrote:
>>> AFAICS it is always a bug to be in a different memory context
>>> in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
>>> robust to not put the burden on the callers.
> 
>> I thought there were comments specifically explaining why it was done
>> that way but I don't recall what they said.
> 
> I think it was just a performance optimization.  It's probably not
> measurable though; even in the in-memory case there's at least a palloc
> inside the put() function, no?

Yes. And many of the callers do the memory context switching dance anyway.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  7. Re: Tuplestore should remember the memory context it's created in

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-22T16:31:03Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> I think it was just a performance optimization.  It's probably not
>> measurable though; even in the in-memory case there's at least a palloc
>> inside the put() function, no?

> Yes. And many of the callers do the memory context switching dance anyway.

Yeah, I was just noticing that.  We should go around and clean those up
if we apply this change.

Looking at the CVS history, I think the reason tuplestore doesn't do its
own memory context switch is that it was cloned from tuplesort, which
didn't either at the time.  But several years ago we changed tuplesort
to be safer about this (it actually keeps its own memory context now),
so it's just inconsistent that tuplestore still exposes the risk.

The ownership business is another story though.  tuplesort doesn't
make any attempt to defend itself against resource-owner changes.  If we
need this for tuplestore I bet we need it for tuplesort too; but do we?

			regards, tom lane

  8. Re: Tuplestore should remember the memory context it's created in

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-22T17:55:22Z

    Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> Patch against CVS HEAD to do that and fix the reported bug attached. Now
>> that the tuplestore_put* switches to the right memory context, we could
>> remove that from all the callers, but this patch only does it for pl_exec.c.
> 
> BTW, I'm not convinced that the owner-switchery you added to pl_exec.c
> is necessary/appropriate.  Under what circumstances would that be a good
> idea?

A PL/pgSQL normally runs in the whatever resource owner is current when
the function is called. When we allocate the tuplestore for return
tuples, it's associated with the current resource owner.

But if you have an exception-block, we start a new subtransaction and
switch to the subtransaction resource owner. If you have a RETURN
NEXT/QUERY in the block, the tuplestore (or the temporary file backing
it, to be precise) is initialized into the subtransaction resource
owner, which is released at subtransaction commit. The subtransaction
resource owner is not the right owner for the tuplestore holding return
tuples.

We already take care to use the right memory context for the tuplestore,
but now that temp files are associated with resource owners, we need to
use the right resource owner as well.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  9. Re: Tuplestore should remember the memory context it's created in

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-22T18:57:00Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> BTW, I'm not convinced that the owner-switchery you added to pl_exec.c
>> is necessary/appropriate.  Under what circumstances would that be a good
>> idea?

> A PL/pgSQL normally runs in the whatever resource owner is current when
> the function is called. When we allocate the tuplestore for return
> tuples, it's associated with the current resource owner.

> But if you have an exception-block, we start a new subtransaction and
> switch to the subtransaction resource owner. If you have a RETURN
> NEXT/QUERY in the block, the tuplestore (or the temporary file backing
> it, to be precise) is initialized into the subtransaction resource
> owner, which is released at subtransaction commit.

Got it.  So doesn't tuplesort have the same issue?

The patch definitely requires more than zero comments.

			regards, tom lane

  10. Re: Tuplestore should remember the memory context it's created in

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-22T20:37:23Z

    Tom Lane wrote:
> Got it.  So doesn't tuplesort have the same issue?

Tuplesort has the same general problem that the caller of puttuple needs
to be in the right resource owner. Which ought to be fixed, especially
since tuplesort doesn't require that for the memory context anymore.

But we don't use tuplesort to return tuples from functions, so it's not
broken in a user-visible way. Unless you can think of another scenario
like that.

> The patch definitely requires more than zero comments.

Sure.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  11. Re: Tuplestore should remember the memory context it's created in

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-22T20:47:38Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> Got it.  So doesn't tuplesort have the same issue?

> Tuplesort has the same general problem that the caller of puttuple needs
> to be in the right resource owner. Which ought to be fixed, especially
> since tuplesort doesn't require that for the memory context anymore.

> But we don't use tuplesort to return tuples from functions, so it's not
> broken in a user-visible way. Unless you can think of another scenario
> like that.

(1) create a cursor whose plan involves a sort that will spill to disk
(2) enter subtransaction
(3) fetch from cursor (causing the sort to actually happen)
(4) leave subtransaction
(5) fetch some more from cursor

Too busy to develop a test case right now, but ISTM it ought to fail.

			regards, tom lane

  12. Re: Tuplestore should remember the memory context it's created in

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-23T09:24:39Z

    Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> But we don't use tuplesort to return tuples from functions, so it's not
>> broken in a user-visible way. Unless you can think of another scenario
>> like that.
> 
> (1) create a cursor whose plan involves a sort that will spill to disk
> (2) enter subtransaction
> (3) fetch from cursor (causing the sort to actually happen)
> (4) leave subtransaction
> (5) fetch some more from cursor
> 
> Too busy to develop a test case right now, but ISTM it ought to fail.

That was exactly the case that we originally fixed, that caused this
PL/pgSQL issue. It works because cursors run within the portal
ResourceOwner.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com