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

On Mon, 10 Feb 2020 at 21:19, Kohei KaiGai <kaigai@heterodb.com> wrote:
>
> 2020年2月10日(月) 13:53 Craig Ringer <craig@2ndquadrant.com>:
> >
> > On Thu, 6 Feb 2020 at 11:09, Andres Freund <andres@anarazel.de> wrote:
> >
> > > 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.
> >
> > Simply using MemoryContext as the NodeTag seems very sensible based on
> > the above discussion.
> >
> > But rather than adding a const char * name to point to some constant
> > for the implementation name as was proposed earlier, I think the
> > existing pointer MemoryContextData->methods is sufficient to identify
> > the context type. We could add a NameData field to
> > MemoryContextMethods that the initializer sets to the implementation
> > name for convenience.
> >
> > It's trivial to see when debugging with a   p ctx->methods->name   .
> > We keep the MemoryContextData size down and we lose nothing. Though
> > gdb is smart enough to annotate a pointer to the symbol
> > AllocSetMethods as such when it sees it in a debuginfo build there's
> > no harm in having a single static string in the const-data segment per
> > memory context type.
> >
> > I'd also like to add a
> >
> >    bool (*instanceof)(MemoryContext context, MemoryContextMethods context_type);
> >
> > to MemoryContextMethods . Then replace all use of   IsA(ctx,
> > AllocSetContext)   etc with a test like:
> >
> >     #define Insanceof_AllocSetContext(ctx) \
> >         (ctx->methods == AllocSetMethods || ctx->is_a(AllocSetMethods));
> >
> AllocSetMethods is statically defined at utils/mmgr/aset.c, so this macro
> shall be available only in this source file.
>
> Isn't it sufficient to have the macro below?
>
> #define Insanceof_AllocSetContext(ctx) \
>   (IsA(ctx, MemoryContext) && \
>    strcmp(((MemoryContext)(ctx))->methods->name, "AllocSetMethods") == 0)
>
> As long as an insane extension does not define a different memory context
> with the same name, it will work.

That wouldn't allow for the sort of extensibility I suggested for
wrapping objects, which is why I thought we might as well ask the
object itself. It's not exactly a new, weird or unusual pattern. A
pointer to AllocSetMethods would need to be made non-static if we
wanted to allow a macro or static inline to avoid the function call
and test it for equality, but that's not IMO a big problem. Or if it
is, well, there's always whole-program optimisation...

Also, isn't strcmp() kinda expensive compared to a simple pointer
value compare anyway? I admit I'm terribly clueless about modern
microarchitectures, so I may be very wrong.

All I'm saying is that if we're changing this, lets learn from what
others have done when writing interfaces and inheritance-type
patterns.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise