Re: Is custom MemoryContext prohibited?

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.


> In other words, we ask the target object what it is.
>
> This would make it possible to create wrapper implementations of
> existing contexts that do extra memory accounting or other limited
> sorts of extensions. The short-circuit testing for the known concrete
> AllocSetMethods should make it pretty much identical in performance
> terms, which is of course rather important.
>
> The OO-alike naming is not a coincidence.
>
> I can't help but notice that we're facing some of the same issues
> faced by early OO patterns. Not too surprising given that Pg uses a
> lot of pseudo-OO - some level of structural inheritance and
> behavioural inheritance, but no encapsulation, no messaging model, no
> code-to-data binding. I'm no OO purist, I don't care much so long as
> it works and is consistent.
>
> In OO terms what we seem to be facing is difficulty with extending
> existing object types into new subtypes without modifying all the code
> that knows how to work with the parent types. MemoryContext is one
> example of this, Node is another. The underlying issue is similar.
>
> Being able to do this is something I'm much more interested in being
> able to do for plan and parse nodes etc than for MemoryContext tbh,
> but the same principles apply.
>
>
> --
>  Craig Ringer                   http://www.2ndQuadrant.com/
>  2ndQuadrant - PostgreSQL Solutions for the Enterprise



--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>