On Tue, Jan 28, 2020 at 11:24 AM Tom Lane <t...@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
v2-0001-Teach-MemoryContext-infrastructure-not-to-depend-.patch
Description: Binary data
