Thinking about inventing MemoryContextSetParent

I'm considering inventing a new mcxt.c primitive,

void MemoryContextSetParent(MemoryContext context, MemoryContext new_parent);

which would have the effect of delinking "context" from its current
parent context and attaching it as a child of the new specified parent.
(Any child contexts that it has would naturally follow along.)
Because of the way that mcxt.c handles parent/child links, there is no
palloc required and so the operation cannot fail.

The use-case that I have for this is to change the status of a memory
context from "temporary" to "permanent". That is, create a context
underneath a transaction-local working context, fill it with stuff
(and/or do other things that might fail), and finally when all is known
good, transfer the context to be a child of CacheMemoryContext. If a
failure happens before that, the context is cleaned up automatically
with no extra effort.

I've spent the past hour or so trying to accomplish the same result by
initially creating the context under CacheMemoryContext and then using
a PG_TRY block to delete it on failure, but that approach is pretty
crufty: it doesn't work nicely when the context has to be passed from
one routine to another, and you end up plastering "volatile" on a lot
of local variables to keep the compiler quiet, and it just seems ugly.
(This is in connection with the long-threatened rewrite of plancache.c:
right now, postgres.c passes a long-lived context to
FastCreateCachedPlan which takes ownership of that, but it has to have
special extra logic to clean up that context on failure, and there is
still a risk of a permanent leak if FastCreateCachedPlan fails partway
through. I'm trying to make that less ugly and more bulletproof.)

Thoughts, objections, bikeshedding?

regards, tom lane

On Sat, Sep 10, 2011 at 06:03:23PM -0400, Tom Lane wrote:
> I'm considering inventing a new mcxt.c primitive,
>
> void MemoryContextSetParent(MemoryContext context, MemoryContext new_parent);
>
> which would have the effect of delinking "context" from its current
> parent context and attaching it as a child of the new specified parent.
> (Any child contexts that it has would naturally follow along.)
> Because of the way that mcxt.c handles parent/child links, there is no
> palloc required and so the operation cannot fail.

I like this idea. Currently the only way to control object lifetime is
at creation time. This means that you can "atomically" change the
lifetime of a collection of object when it reaches a state we like.

It occured to me this might be useful in other places where we copy
data into a longer lived context after checking. Maybe config file
reading or plan construction. The only issue I can think of is if
people where allocating in the local context assuming it would be
cleaned up and this data got kept as well. So it's probably not
appropriate for things that happen really often.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

Excerpts from Tom Lane's message of sáb sep 10 19:03:23 -0300 2011:
> I'm considering inventing a new mcxt.c primitive,
>
> void MemoryContextSetParent(MemoryContext context, MemoryContext new_parent);
>
> which would have the effect of delinking "context" from its current
> parent context and attaching it as a child of the new specified parent.
> (Any child contexts that it has would naturally follow along.)
> Because of the way that mcxt.c handles parent/child links, there is no
> palloc required and so the operation cannot fail.

Interesting. I wonder whether we could use this somehow to fix
performance problems in certain subtransaction code paths that "reassign
stuff to the parent"; instead of moving pointers or memory around,
perhaps we could do something like this. Not that I have actually
looked into it.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Tom Lane's message of sb sep 10 19:03:23 -0300 2011:
>> I'm considering inventing a new mcxt.c primitive,
>>
>> void MemoryContextSetParent(MemoryContext context, MemoryContext new_parent);
>>
>> which would have the effect of delinking "context" from its current
>> parent context and attaching it as a child of the new specified parent.
>> (Any child contexts that it has would naturally follow along.)
>> Because of the way that mcxt.c handles parent/child links, there is no
>> palloc required and so the operation cannot fail.

> Interesting. I wonder whether we could use this somehow to fix
> performance problems in certain subtransaction code paths that "reassign
> stuff to the parent"; instead of moving pointers or memory around,
> perhaps we could do something like this. Not that I have actually
> looked into it.

Yeah, I think it would be worth looking for places where we are either
copying lots-o-stuff or else jumping through weird hoops to avoid doing
that. I'm about halfway through rewriting the plancache and SPIPlan
stuff using this mechanism, and it seems to be coming out a whole lot
nicer --- it'll probably end up with less parse-tree-copying overall,
and much less risk of leaking memory when you hit an error partway
through constructing a cached plan. (The existing SPI code gets a
completely failing grade on that aspect :-(.)

regards, tom lane