On Tue, Jan 28, 2020 at 11:24 AM Tom Lane <tgl@sss.pgh.pa.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, and possibly on the grounds that
somebody might be examining the initial bytes manually to try to
figure out what they've got, and having the magic number there makes
it easier. Regarding space consumption, I guess this structure is
already over 64 bytes and not close to 128 bytes, so adding another 8
bytes probably isn't very meaningful, but I don't love it. One thing
that concerns me a bit is that if somebody adds their own type of
memory context, then MemoryContextType will have a value that is not
actually in the enum. If compilers are optimizing the code on the
assumption that this cannot occur, do we need to worry about undefined
behavior?
Actually, I have what I think is a better idea. I notice that the
"type" field is actually completely unused. We initialize it, and then
nothing in the code ever checks it or relies on it for any purpose.
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. 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.
Here's a v2 that approaches the problem that way.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company