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

Hi,

On 2020-02-05 09:28:08 -0500, Robert Haas wrote:
> On Mon, Feb 3, 2020 at 7:26 PM Andres Freund <andres@anarazel.de> wrote:
> > Hm. I kinda like the idea of still having one NodeTag identifying memory
> > contexts, and then some additional field identifying the actual
> > type. Being able to continue to rely on IsA() etc imo is nice.  I think
> > nodes.h itself only would be a problem for frontend code because we put
> > a lot of other stuff too. We should just separate the actually generic
> > stuff out. I think it's going to be like 2 seconds once we have memory
> > contexts until we're e.g. going to want to also have pg_list.h - which
> > is harder without knowing the tags.
> 
> The problem with IsA() is that it assumes that you've got all the node
> tags that can ever exist in one big enum. I don't see how to make that
> work once you extend the system to work with more than one program. I
> think it will be really confusing if frontend code starts reusing
> random backend data structures. Like, fundamental things like List,
> sure, that should be exposed. But if people start creating Plan or FDW
> objects in the frontend, it's just going to be chaos. And I don't
> think we want new objects that people may add for frontend code to be
> visible to backend code, either.

I wasn't advocating for making plannodes.h etc frontend usable. I think
that's a fairly different discussion than making enum NodeTag,
pg_list.h, memutils.h available.  I don't see them having access to the
numerical value of node tag for backend structs as something actually
problematic (I'm pretty sure you can do that today already if you really
want to - but why would you?).

I don't buy that having a separate magic number for various types that
we may want to use both frontend and backend is better than largely just
having one set of such magic type identifiers.


> > It seems like a good idea to still have an additional identifier for
> > each node type, for some cross checking. How about just frobbing the
> > pointer to the MemoryContextMethod slightly, and storing that in an
> > additional field? That'd be something fairly unlikely to ever be a false
> > positive, and it doesn't require dereferencing any additional memory.
> 
> That would be fine as an internal sanity check, but if Tom is unhappy
> with the idea of having to try to make sense of a function pointer,
> he's probably going to be even less happy about trying to make sense
> of a frobbed pointer. And I would actually agree with him on that
> point.

I feel the concern about identifying nodes can pretty readily be
addressed by adding a name to the context methods - something that's
useful independently too. It'd e.g. be nice to have some generic print
routines for memory context stats.


> I think we're all pursuing slightly different goals here. KaiGai's
> main goal is to make it possible for third-party code to add new kinds
> of memory contexts. My main goal is to make memory contexts not depend
> on backend-only infrastructure. Tom is concerned about debuggability.
> Your concern here is about sanity checking. There's some overlap
> between those goals but the absolute best thing for any given one of
> them might be really bad for one of the other ones; hopefully we can
> find some compromise that gets everybody the things they care about
> most.

FWIW, I care most about the frontend usable bit too. I was bringing up
the cross check idea solely as an idea of how to not loose, but if
anything, improve our error checking, while making things more
extensible.

Greetings,

Andres Freund