Re: MemoryContext reset/delete callbacks - Mailing list pgsql-hackers

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