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

Hi,

On 2020-02-05 10:56:42 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Feb 5, 2020 at 10:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> an enum field right in the context.  You'll have to do an extra step to
> >> discover the context's type, and if you jump to the wrong conclusion
> >> and do, say,
> >>     p *(AllocSetContext *) ptr_value
> >> when it's really some other context type, there won't be anything
> >> as obvious as "type = T_GenerationContext" in what is printed to
> >> tell you you were wrong.
> 
> > Doesn't the proposed magic number address this concern?
> 
> No, because (a) it will be a random magic number that nobody will
> remember, and gdb won't print in any helpful form; (b) at least
> as I understood the proposal, there'd be just one magic number for
> all types of memory context.

I still don't get what reason there is to not use T_MemoryContext as the
magic number, instead of something randomly new. It's really not
problematic to expose those numerical values. And it means that the
first bit of visual inspection is going to be the same as it always has
been, and the same as it works for most other types one regularly
inspects in postgres.

What about using T_MemoryContext as the identifier that's the same for
all types of memory contexts and additionally have a new 'const char
*contexttype' in MemoryContextData, that points to
MemoryContextMethods.contexttype (which is a char[32] or such).  As it's
a char * debuggers will display the value, making it easy to identify
the specific type. And sure, it's 8 additional bytes instead of 4 - but
I don't see that being a problem.

And because contexttype points into a specific offset in the
MemoryContextData, we can use that as a crosscheck, by Asserting that
MemoryContext->methods + offsetof(MemoryContextMethods.contexttype) == MemoryContext->contexttype


> Another issue with relying on only a magic number is that if you
> get confused and do "p *(AllocSetContext *) ptr_value" on something
> that doesn't point at any sort of memory context at all, there will
> not be *anything* except confusing field values to help you realize
> that.  One of the great advantages of the Node system, IME, is that
> when you try to print out a Node-subtype struct, the first field
> in what is printed is either the Node type you were expecting, or
> some recognizable other Node code, or obvious garbage.

I agree that that's good - which is why I think we should simply not
give it up here.

Greetings,

Andres Freund