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
