2020年1月29日(水) 0:56 Tomas Vondra <tomas.von...@2ndquadrant.com>: > > On Tue, Jan 28, 2020 at 11:32:49PM +0900, Kohei KaiGai wrote: > >2020年1月28日(火) 23:09 Tomas Vondra <tomas.von...@2ndquadrant.com>: > >> > >> On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote: > >> >Hello, > >> > > >> >I noticed MemoryContextIsValid() called by various kinds of memory context > >> >routines checks its node-tag as follows: > >> > > >> >#define MemoryContextIsValid(context) \ > >> > ((context) != NULL && \ > >> > (IsA((context), AllocSetContext) || \ > >> > IsA((context), SlabContext) || \ > >> > IsA((context), GenerationContext))) > >> > > >> >It allows only "known" memory context methods, even though the memory > >> >context > >> >mechanism enables to implement custom memory allocator by extensions. > >> >Here is a node tag nobody used: T_MemoryContext. > >> >It looks to me T_MemoryContext is a neutral naming for custom memory > >> >context, > >> >and here is no reason why memory context functions prevents custom > >> >methods. > >> > > >> > >> Good question. I don't think there's an explicit reason not to allow > >> extensions to define custom memory contexts, and using T_MemoryContext > >> seems like a possible solution. It's a bit weird though, because all the > >> actual contexts are kinda "subclasses" of MemoryContext. So maybe adding > >> T_CustomMemoryContext would be a better choice, but that only works in > >> master, of course. > >> > >From the standpoint of extension author, reuse of T_MemoryContext and > >back patching the change on MemoryContextIsValid() makes us happy. :) > >However, even if we add a new node-tag here, the custom memory-context > >can leave to use fake node-tag a few years later. It's better than nothing. > > > > Oh, right. I forgot we still need to backpatch this bit. But that seems > like a fairly small amount of code, so it should work. > > I think we can't backpatch the addition of T_CustomMemoryContext anyway > as it essentially breaks ABI, as it changes the values assigned to T_ > constants. > > >> Also, it won't work if we need to add memory contexts to equalfuncs.c > >> etc. but maybe won't need that - it's more a theoretical issue. > >> > >Right now, none of nodes/XXXfuncs.c support these class of nodes related to > >MemoryContext. It shall not be a matter. > > > > Yes. I did not really mean it as argument against the patch, it was > meant more like "This could be an issue, but it actually is not." Sorry > if that wasn't clear. > > >> >https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243 > >> >I recently implemented a custom memory context for shared memory > >> >allocation > >> >with portable pointers. It shall be used for cache of pre-built gpu > >> >binary code and > >> >metadata cache of apache arrow files. > >> >However, the assertion check above requires extension to set a fake > >> >node-tag > >> >to avoid backend crash. Right now, it is harmless to set > >> >T_AllocSetContext, but > >> >feel a bit bad. > >> > > >> > >> Interesting. Does that mean the hared memory contexts are part of the > >> same hierarchy as "normal" contexts? That would be a bit confusing, I > >> think. > >> > >If this shared memory context is a child of normal context, likely, it > >should be > >reset or deleted as usual. However, if this shared memory context performs > >as a parent of normal context, it makes a problem when different process > >tries > >to delete this context, because its child (normal context) exists at the > >creator > >process only. So, it needs to be used carefully. > > > > Yeah, handling life cycle of a mix of those contexts may be quite tricky. > > But my concern was a bit more general - is it a good idea to hide the > nature of the memory context behind the same API. If you call palloc() > shouldn't you really know whether you're allocating the stuff in regular > or shared memory context? > > Maybe it makes perfect sense, but my initial impression is that those > seem rather different, so maybe we should keep them separate (in > separate hierarchies or something). But I admit I don't have much > experience with use cases that would require such shmem contexts. > Yeah, as you mentioned, we have no way to distinguish whether a particular memory chunk is private memory or shared memory, right now. It is the responsibility of software developers, and I assume the shared- memory chunks are applied on a limited usages where it has a certain reason why it should be shared. On the other hand, it is the same situation even if private memory. We should pay attention to the memory context to allocate a memory chunk from. For example, state object to be valid during query execution must be allocated from estate->es_query_cxt. If someone allocates it from CurrentMemoryContext, then implicitly released, we shall consider it is a bug.
Thanks, -- HeteroDB, Inc / The PG-Strom Project KaiGai Kohei <kai...@heterodb.com>