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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to