Re: MemoryContext reset/delete callbacks

Hi,

On 2015-02-26 19:28:50 -0500, Tom Lane wrote:
> We discussed this idea a couple weeks ago.

Hm, didn't follow that discussion.

> The core of it is that when a memory context is being deleted, you
> might want something extra to happen beyond just pfree'ing everything
> in the context.

I've certainly wished for this a couple times...

> Attached is a draft patch for this. I've not really tested it more than
> seeing that it compiles, but it's so simple that there are unlikely to be
> bugs as such in it. There are some design decisions that could be
> questioned though:
>
> 1. I used ilists for the linked list of callback requests. This creates a
> dependency from memory contexts to lib/ilist. That's all right at the
> moment since lib/ilist does no memory allocation, but it might be
> logically problematic. We could just use explicit "struct foo *" links
> with little if any notational penalty, so I wonder if that would be
> better.

Maybe I'm partial here, but I don't see a problem. Actually the reason I
started the ilist stuff was that I wrote a different memory context
implementation ;). Wish I'd time/energy to go back to that...

> 2. I specified that callers have to allocate the storage for the callback
> structs. This saves a palloc cycle in just about every use-case I've
> thought of, since callers generally will need to palloc some larger struct
> of their own and they can just embed the MemoryContextCallback struct in
> that. It does seem like this offers a larger surface for screwups, in
> particular putting the callback struct in the wrong memory context --- but
> there's plenty of surface for that type of mistake anyway, if you put
> whatever the "void *arg" is pointing at in the wrong context.
> So I'm OK with this but could also accept a design in which
> MemoryContextRegisterResetCallback takes just a function pointer and a
> "void *arg" and palloc's the callback struct for itself in the target
> context. I'm unsure whether that adds enough safety to justify a separate
> palloc.

I'm unworried about this. Yes, one might be confused for a short while,
but compared to the complexity of using any such facility sanely it
seems barely relevant.

> 3. Is the "void *arg" API for the callback func sufficient? I considered
> also passing it the MemoryContext, but couldn't really find a use-case
> to justify that.

Hm, seems sufficient to me.

> 4. I intentionally didn't provide a MemoryContextDeregisterResetCallback
> API. Since it's just a singly-linked list, that could be an expensive
> operation and so I'd rather discourage it. In the use cases I've thought
> of, you'd want the callback to remain active for the life of the context
> anyway, so there would be no need. (And, of course, there is slist_delete
> for the truly determined ...)

Yea, that seems fine. If you don't want the callback to do anything
anymore, it's easy enough to just set a flag somewhere.

> /*
> *************** typedef struct MemoryContextData
> *** 59,72 ****
> MemoryContext firstchild; /* head of linked list of children */
> MemoryContext nextchild; /* next child of same parent */
> char *name; /* context name (just for debugging) */
> bool isReset; /* T = no space alloced since last reset */
> #ifdef USE_ASSERT_CHECKING
> ! bool allowInCritSection; /* allow palloc in critical section */
> #endif
> } MemoryContextData;

It's a bit sad to push AllocSetContextData onto four cachelines from the
current three... That stuff is hot. But I don't really see a way around
it right now. And it seems like it'd give us more amunition to improve
things than the small loss of speed it implies.

I guess it might needa a warning about being careful what you directly
free if you use this. Might also better fit in a layer above this...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services