Re: Is custom MemoryContext prohibited? - Mailing list pgsql-hackers

On Tue, Jan 28, 2020 at 11:32:49PM +0900, Kohei KaiGai wrote:
>2020年1月28日(火) 23:09 Tomas Vondra <tomas.vondra@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.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services