Re: Is custom MemoryContext prohibited? - Mailing list pgsql-hackers

On Tue, Jan 28, 2020 at 1:08 PM Tom Lane <tgl@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