2020年1月29日(水) 2:18 Robert Haas <robertmh...@gmail.com>: > > 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. > How about to have "const char *name" in MemoryContextMethods? It is more human readable for debugging, than raw function pointers. We already have similar field to identify the methods at CustomScanMethods. (it is also used for EXPLAIN, not only debugging...)
I love the idea to identify the memory-context type with single identifiers rather than two. If we would have sub-field Id and memory-context methods separately, it probably needs Assert() to check the consistency of them, isn't it? Thanks, -- HeteroDB, Inc / The PG-Strom Project KaiGai Kohei <kai...@heterodb.com>
