On Tue, Jan 28, 2020 at 1:08 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > 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.
I half-expected this reaction, but I think it's unjustified. Two sources of truth are not better than one, and I don't think that any other place where we use a vtable-type approach includes a redundant type field just for decoration. Can you think of a counterexample? After scrounging around the source tree a bit, the most direct parallel I can find is probably TupleTableSlot, which contains a pointer to a TupleTableSlotOps, but not a separate field identifying the slot type. I don't remember you or anybody objecting that TupleTableSlot should contain both "const TupleTableSlotOps *const tts_ops" and also "enum TupleTableSlotType", and I don't think that such a suggestion would have been looked upon very favorably, not only because it would have made the struct bigger and served no necessary purpose, but also because having a centralized list of all TupleTableSlot types flies in the face of the essential goal of the table AM interface, which is to allow adding new table type (and a slot type for each) without having to modify core code. That exact consideration is also relevant here: KaiGai wants to be able to add his own memory context type in third-party code without having to modify core code. I've wanted to do that in the past, too. Having to list all the context types in an enum means that you really can't do that, which sucks, unless you're willing to lie about the context type and hope that nobody adds code that cares about it. Is there an alternate solution that you can propose that does not prevent that? You might be entirely correct that there are some debuggers that can't print function pointers correctly. I have not run across one, if at all, in a really long time, but I also work mostly on MacOS and Linux these days, and those are pretty mainstream platforms where such problems are less likely. However, I suspect that there are very few developers who do the bulk of their work on obscure platforms with poorly-functioning debuggers. The only time it's likely to come up is if a buggy commit makes things crash on some random buildfarm critter and we need to debug from a core dump or whatever. But, if that does happen, we're not dead. The only possible values of the "methods" pointer -- if only core code is at issue -- are &AllocSetMethods, &SlabMethods, and &GenerationMethods, so somebody can just print out those values and compare it to the pointer they find. That is a lot less convenient than being able to just print context->methods[0] and see everything, but if it only comes up when debugging irreproducible crashes on obscure platforms, it seems OK to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
