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

2020年1月29日(水) 0:56 Tomas Vondra <tomas.vondra@2ndquadrant.com>:
>
> 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.
>
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 <kaigai@heterodb.com>