Tomas Vondra <tomas.von...@2ndquadrant.com> writes:
> On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote:
>> 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)))
> 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.
I think the original reason for having distinct node types was to let
individual mcxt functions verify that they were passed the right type
of node ... but I don't see any of them actually doing so, and the
context-methods API makes it a bit hard to credit that we need to.
So there's certainly a case for replacing all three node typecodes
with just MemoryContext. On the other hand, it'd be ugly and it would
complicate debugging: you could not be totally sure which struct type
to cast a pointer-to-MemoryContext to when trying to inspect the
contents. We have a roughly comparable situation with respect to
Value --- the node type codes we use with it, such as T_String, don't
have anything to do with the struct typedef name. I've always found
that very confusing when debugging. When gdb tells me that
"*(Node*) address" is T_String, the first thing I want to do is write
"p *(String*) address" and of course that doesn't work.
I don't actually believe that private context types in extensions is
a very likely use-case, so on the whole I'd just as soon leave this
alone. If we did want to do something, I'd vote for one NodeTag
code T_MemoryContext and then a secondary ID field that's an enum
over specific memory context types. But that doesn't really improve
matters for debugging extension contexts, because they still don't
have a way to add elements to the secondary enum.
regards, tom lane
