Re: Is custom MemoryContext prohibited?

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jan 28, 2020 at 11:24 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I strongly object to having the subtype field be just "char".
>> I want it to be declared "MemoryContextType", so that gdb will
>> still be telling me explicitly what struct type this really is.
>> I realize that you probably did that for alignment reasons, but
>> maybe we could shave the magic number down to 2 bytes, and then
>> rearrange the field order? Or just not sweat so much about
>> wasting a longword here. Having those bools up at the front
>> is pretty ugly anyway.

> I kind of dislike having the magic number not be the first thing in
> the struct on aesthetic grounds,

Huh? I didn't propose that. I was thinking either

uint16 magic;
bool isReset;
bool allowInCritSection;
enum type;
... 64-bit fields follow ...

or

uint32 magic;
enum type;
bool isReset;
bool allowInCritSection;
... 64-bit fields follow ...

where the latter wastes space unless the compiler chooses to fit the
enum into 16 bits, but it's not really our fault if it doesn't. Besides,
what's the reason to think we'll never add any more bools here? I don't
think we need to be that excited about the padding.

> So, we could have a bug in the code that initializes that field with
> the wrong value, for a new context type or in general, and only a
> developer with a debugger would ever notice.

Right, but that is a pretty important use-case.

> That's because the thing
> that really matters is the 'methods' array. So what I propose is that
> we nuke the type field from orbit. If a developer wants to figure out
> what type of context they've got, they should print
> context->methods[0]; gdb will tell you the function names stored
> there, and then you'll know *for real* what type of context you've
> got.

No. No. Just NO. (A) that's overly complex for developers to use,
and (B) you have far too much faith in the debugger producing something
useful. (My experience is that it'll fail to render function pointers
legibly on an awful lot of platforms.) Plus, you won't actually save
any space by removing both of those fields.

If we were going to conclude that we don't really need a magic number,
I'd opt for replacing the NodeTag with an enum MemoryContextType field
that's decoupled from NodeTag. But I don't feel tremendously happy
about not having a magic number. That'd make it noticeably harder
to recognize cases where you're referencing an invalid context pointer.

In the end, trying to shave a couple of bytes from context headers
seems pretty penny-wise and pound-foolish. There are lots of other
structs with significantly higher usage where we've not stopped to
worry about alignment padding, so why here? Personally I'd just
put the bools back at the end where they used to be.

regards, tom lane