2020年1月29日(水) 2:18 Robert Haas <robertmhaas@gmail.com>:
>
> 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.
>
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 <kaigai@heterodb.com>
