Is custom MemoryContext prohibited?

On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote:

Hello,

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:

#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

It allows only "known" memory context methods, even though the memory context
mechanism enables to implement custom memory allocator by extensions.
Here is a node tag nobody used: T_MemoryContext.
It looks to me T_MemoryContext is a neutral naming for custom memory context,
and here is no reason why memory context functions prevents custom methods.

Good question. I don't think there's an explicit reason not to allow
extensions to define custom memory contexts, and using T_MemoryContext
seems like a possible solution. It's a bit weird though, because all the
actual contexts are kinda "subclasses" of MemoryContext. So maybe adding
T_CustomMemoryContext would be a better choice, but that only works in
master, of course.

Also, it won't work if we need to add memory contexts to equalfuncs.c
etc. but maybe won't need that - it's more a theoretical issue.

https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243
I recently implemented a custom memory context for shared memory allocation
with portable pointers. It shall be used for cache of pre-built gpu
binary code and
metadata cache of apache arrow files.
However, the assertion check above requires extension to set a fake node-tag
to avoid backend crash. Right now, it is harmless to set T_AllocSetContext, but
feel a bit bad.

Interesting. Does that mean the hared memory contexts are part of the
same hierarchy as "normal" contexts? That would be a bit confusing, I
think.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

2020年1月28日(火) 23:09 Tomas Vondra <tomas.vondra@2ndquadrant.com>:

On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote:

Hello,

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:

#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

It allows only "known" memory context methods, even though the memory context
mechanism enables to implement custom memory allocator by extensions.
Here is a node tag nobody used: T_MemoryContext.
It looks to me T_MemoryContext is a neutral naming for custom memory context,
and here is no reason why memory context functions prevents custom methods.

Good question. I don't think there's an explicit reason not to allow
extensions to define custom memory contexts, and using T_MemoryContext
seems like a possible solution. It's a bit weird though, because all the
actual contexts are kinda "subclasses" of MemoryContext. So maybe adding
T_CustomMemoryContext would be a better choice, but that only works in
master, of course.

From the standpoint of extension author, reuse of T_MemoryContext and
back patching the change on MemoryContextIsValid() makes us happy. :)
However, even if we add a new node-tag here, the custom memory-context
can leave to use fake node-tag a few years later. It's better than nothing.

Also, it won't work if we need to add memory contexts to equalfuncs.c
etc. but maybe won't need that - it's more a theoretical issue.

Right now, none of nodes/XXXfuncs.c support these class of nodes related to
MemoryContext. It shall not be a matter.

https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243
I recently implemented a custom memory context for shared memory allocation
with portable pointers. It shall be used for cache of pre-built gpu
binary code and
metadata cache of apache arrow files.
However, the assertion check above requires extension to set a fake node-tag
to avoid backend crash. Right now, it is harmless to set T_AllocSetContext, but
feel a bit bad.

Interesting. Does that mean the hared memory contexts are part of the
same hierarchy as "normal" contexts? That would be a bit confusing, I
think.

If this shared memory context is a child of normal context, likely, it should be
reset or deleted as usual. However, if this shared memory context performs
as a parent of normal context, it makes a problem when different process tries
to delete this context, because its child (normal context) exists at the creator
process only. So, it needs to be used carefully.

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

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:

On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote:

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:
#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

Good question. I don't think there's an explicit reason not to allow
extensions to define custom memory contexts, and using T_MemoryContext
seems like a possible solution. It's a bit weird though, because all the
actual contexts are kinda "subclasses" of MemoryContext. So maybe adding
T_CustomMemoryContext would be a better choice, but that only works in
master, of course.

I think the original reason for having distinct node types was to let
individual mcxt functions verify that they were passed the right type
of node ... but I don't see any of them actually doing so, and the
context-methods API makes it a bit hard to credit that we need to.
So there's certainly a case for replacing all three node typecodes
with just MemoryContext. On the other hand, it'd be ugly and it would
complicate debugging: you could not be totally sure which struct type
to cast a pointer-to-MemoryContext to when trying to inspect the
contents. We have a roughly comparable situation with respect to
Value --- the node type codes we use with it, such as T_String, don't
have anything to do with the struct typedef name. I've always found
that very confusing when debugging. When gdb tells me that
"*(Node*) address" is T_String, the first thing I want to do is write
"p *(String*) address" and of course that doesn't work.

I don't actually believe that private context types in extensions is
a very likely use-case, so on the whole I'd just as soon leave this
alone. If we did want to do something, I'd vote for one NodeTag
code T_MemoryContext and then a secondary ID field that's an enum
over specific memory context types. But that doesn't really improve
matters for debugging extension contexts, because they still don't
have a way to add elements to the secondary enum.

regards, tom lane

On Tue, Jan 28, 2020 at 11:32:49PM +0900, Kohei KaiGai wrote:

2020年1月28日(火) 23:09 Tomas Vondra <tomas.vondra@2ndquadrant.com>:

On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote:

Hello,

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:

#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

It allows only "known" memory context methods, even though the memory context
mechanism enables to implement custom memory allocator by extensions.
Here is a node tag nobody used: T_MemoryContext.
It looks to me T_MemoryContext is a neutral naming for custom memory context,
and here is no reason why memory context functions prevents custom methods.

Good question. I don't think there's an explicit reason not to allow
extensions to define custom memory contexts, and using T_MemoryContext
seems like a possible solution. It's a bit weird though, because all the
actual contexts are kinda "subclasses" of MemoryContext. So maybe adding
T_CustomMemoryContext would be a better choice, but that only works in
master, of course.

From the standpoint of extension author, reuse of T_MemoryContext and
back patching the change on MemoryContextIsValid() makes us happy. :)
However, even if we add a new node-tag here, the custom memory-context
can leave to use fake node-tag a few years later. It's better than nothing.

Oh, right. I forgot we still need to backpatch this bit. But that seems
like a fairly small amount of code, so it should work.

I think we can't backpatch the addition of T_CustomMemoryContext anyway
as it essentially breaks ABI, as it changes the values assigned to T_
constants.

Also, it won't work if we need to add memory contexts to equalfuncs.c
etc. but maybe won't need that - it's more a theoretical issue.

Right now, none of nodes/XXXfuncs.c support these class of nodes related to
MemoryContext. It shall not be a matter.

Yes. I did not really mean it as argument against the patch, it was
meant more like "This could be an issue, but it actually is not." Sorry
if that wasn't clear.

https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243
I recently implemented a custom memory context for shared memory allocation
with portable pointers. It shall be used for cache of pre-built gpu
binary code and
metadata cache of apache arrow files.
However, the assertion check above requires extension to set a fake node-tag
to avoid backend crash. Right now, it is harmless to set T_AllocSetContext, but
feel a bit bad.

Interesting. Does that mean the hared memory contexts are part of the
same hierarchy as "normal" contexts? That would be a bit confusing, I
think.

If this shared memory context is a child of normal context, likely, it should be
reset or deleted as usual. However, if this shared memory context performs
as a parent of normal context, it makes a problem when different process tries
to delete this context, because its child (normal context) exists at the creator
process only. So, it needs to be used carefully.

Yeah, handling life cycle of a mix of those contexts may be quite tricky.

But my concern was a bit more general - is it a good idea to hide the
nature of the memory context behind the same API. If you call palloc()
shouldn't you really know whether you're allocating the stuff in regular
or shared memory context?

Maybe it makes perfect sense, but my initial impression is that those
seem rather different, so maybe we should keep them separate (in
separate hierarchies or something). But I admit I don't have much
experience with use cases that would require such shmem contexts.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 28, 2020 at 10:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't actually believe that private context types in extensions is
a very likely use-case, so on the whole I'd just as soon leave this
alone. If we did want to do something, I'd vote for one NodeTag
code T_MemoryContext and then a secondary ID field that's an enum
over specific memory context types.

I generally like this idea, but I'd like to propose that we instead
replace the NodeTag with a 4-byte magic number. I was studying how
feasible it would be to make memory contexts available in frontend
code, and it doesn't look all that bad, but one of the downsides is
that nodes/memnodes.h includes nodes/nodes.h, and I think it would not
be a good idea to make frontend code depend on nodes/nodes.h, which
seems like it's really a backend-only piece of infrastructure. Using a
magic number would allow us to avoid that, and it doesn't really cost
anything, because the memory context nodes really don't participate in
any of that infrastructure anyway.

Along with that, I think we could also change MemoryContextIsValid()
to just check the magic number and not validate the type field. I
think the spirit of the code is just to check that we've got some kind
of memory context rather than random garbage in memory, and checking
the magic number is good enough for that. It's possibly a little
better than what we have now, since a node tag is a small integer,
which might be more likely to occur at a random pointer address. At
any rate I think it's no worse.

Proposed patch attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jan 28, 2020 at 10:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't actually believe that private context types in extensions is
a very likely use-case, so on the whole I'd just as soon leave this
alone. If we did want to do something, I'd vote for one NodeTag
code T_MemoryContext and then a secondary ID field that's an enum
over specific memory context types.

I generally like this idea, but I'd like to propose that we instead
replace the NodeTag with a 4-byte magic number.

Yeah, there's something to be said for that. It's unlikely that it'd
ever make sense for us to have copy/equal/write/read/etc support for
memory context headers, so having them be part of the Node taxonomy
doesn't seem very necessary.

Along with that, I think we could also change MemoryContextIsValid()
to just check the magic number and not validate the type field.

Right, that's isomorphic to what I was imagining: there'd be just
one check not N.

Proposed patch attached.

I strongly object to having the subtype field be just "char".
I want it to be declared "MemoryContextType", so that gdb will
still be telling me explicitly what struct type this really is.
I realize that you probably did that for alignment reasons, but
maybe we could shave the magic number down to 2 bytes, and then
rearrange the field order? Or just not sweat so much about
wasting a longword here. Having those bools up at the front
is pretty ugly anyway.

regards, tom lane

On Tue, Jan 28, 2020 at 11:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I strongly object to having the subtype field be just "char".
I want it to be declared "MemoryContextType", so that gdb will
still be telling me explicitly what struct type this really is.
I realize that you probably did that for alignment reasons, but
maybe we could shave the magic number down to 2 bytes, and then
rearrange the field order? Or just not sweat so much about
wasting a longword here. Having those bools up at the front
is pretty ugly anyway.

I kind of dislike having the magic number not be the first thing in
the struct on aesthetic grounds, and possibly on the grounds that
somebody might be examining the initial bytes manually to try to
figure out what they've got, and having the magic number there makes
it easier. Regarding space consumption, I guess this structure is
already over 64 bytes and not close to 128 bytes, so adding another 8
bytes probably isn't very meaningful, but I don't love it. One thing
that concerns me a bit is that if somebody adds their own type of
memory context, then MemoryContextType will have a value that is not
actually in the enum. If compilers are optimizing the code on the
assumption that this cannot occur, do we need to worry about undefined
behavior?

Actually, I have what I think is a better idea. I notice that the
"type" field is actually completely unused. We initialize it, and then
nothing in the code ever checks it or relies on it for any purpose.
So, we could have a bug in the code that initializes that field with
the wrong value, for a new context type or in general, and only a
developer with a debugger would ever notice. 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.

Here's a v2 that approaches the problem that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Jan 28, 2020 at 11:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I strongly object to having the subtype field be just "char".
I want it to be declared "MemoryContextType", so that gdb will
still be telling me explicitly what struct type this really is.
I realize that you probably did that for alignment reasons, but
maybe we could shave the magic number down to 2 bytes, and then
rearrange the field order? Or just not sweat so much about
wasting a longword here. Having those bools up at the front
is pretty ugly anyway.

I kind of dislike having the magic number not be the first thing in
the struct on aesthetic grounds,

Huh? I didn't propose that. I was thinking either

uint16 magic;
bool isReset;
bool allowInCritSection;
enum type;
... 64-bit fields follow ...

or

uint32 magic;
enum type;
bool isReset;
bool allowInCritSection;
... 64-bit fields follow ...

where the latter wastes space unless the compiler chooses to fit the
enum into 16 bits, but it's not really our fault if it doesn't. Besides,
what's the reason to think we'll never add any more bools here? I don't
think we need to be that excited about the padding.

So, we could have a bug in the code that initializes that field with
the wrong value, for a new context type or in general, and only a
developer with a debugger would ever notice.

Right, but that is a pretty important use-case.

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.

If we were going to conclude that we don't really need a magic number,
I'd opt for replacing the NodeTag with an enum MemoryContextType field
that's decoupled from NodeTag. But I don't feel tremendously happy
about not having a magic number. That'd make it noticeably harder
to recognize cases where you're referencing an invalid context pointer.

In the end, trying to shave a couple of bytes from context headers
seems pretty penny-wise and pound-foolish. There are lots of other
structs with significantly higher usage where we've not stopped to
worry about alignment padding, so why here? Personally I'd just
put the bools back at the end where they used to be.

regards, tom lane

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

On Tue, Jan 28, 2020 at 2:55 PM Kohei KaiGai <kaigai@heterodb.com> wrote:

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:

#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

It allows only "known" memory context methods, even though the memory context
mechanism enables to implement custom memory allocator by extensions.
Here is a node tag nobody used: T_MemoryContext.
It looks to me T_MemoryContext is a neutral naming for custom memory context,
and here is no reason why memory context functions prevents custom methods.

https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243
I recently implemented a custom memory context for shared memory allocation
with portable pointers. It shall be used for cache of pre-built gpu
binary code and
metadata cache of apache arrow files.
However, the assertion check above requires extension to set a fake node-tag
to avoid backend crash. Right now, it is harmless to set T_AllocSetContext, but
feel a bit bad.

FWIW the code in https://commitfest.postgresql.org/26/2325/ ran into
exactly the same problem while making nearly exactly the same kind of
thing (namely, a MemoryContext backed by space in the main shm area,
in this case reusing the dsa.c allocator).

2020年1月29日(水) 0:56 Tomas Vondra <tomas.vondra@2ndquadrant.com>:

On Tue, Jan 28, 2020 at 11:32:49PM +0900, Kohei KaiGai wrote:

2020年1月28日(火) 23:09 Tomas Vondra <tomas.vondra@2ndquadrant.com>:

On Tue, Jan 28, 2020 at 10:55:11AM +0900, Kohei KaiGai wrote:

Hello,

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:

#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

It allows only "known" memory context methods, even though the memory context
mechanism enables to implement custom memory allocator by extensions.
Here is a node tag nobody used: T_MemoryContext.
It looks to me T_MemoryContext is a neutral naming for custom memory context,
and here is no reason why memory context functions prevents custom methods.

Good question. I don't think there's an explicit reason not to allow
extensions to define custom memory contexts, and using T_MemoryContext
seems like a possible solution. It's a bit weird though, because all the
actual contexts are kinda "subclasses" of MemoryContext. So maybe adding
T_CustomMemoryContext would be a better choice, but that only works in
master, of course.

From the standpoint of extension author, reuse of T_MemoryContext and
back patching the change on MemoryContextIsValid() makes us happy. :)
However, even if we add a new node-tag here, the custom memory-context
can leave to use fake node-tag a few years later. It's better than nothing.

Oh, right. I forgot we still need to backpatch this bit. But that seems
like a fairly small amount of code, so it should work.

I think we can't backpatch the addition of T_CustomMemoryContext anyway
as it essentially breaks ABI, as it changes the values assigned to T_
constants.

Also, it won't work if we need to add memory contexts to equalfuncs.c
etc. but maybe won't need that - it's more a theoretical issue.

Right now, none of nodes/XXXfuncs.c support these class of nodes related to
MemoryContext. It shall not be a matter.

Yes. I did not really mean it as argument against the patch, it was
meant more like "This could be an issue, but it actually is not." Sorry
if that wasn't clear.

https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243
I recently implemented a custom memory context for shared memory allocation
with portable pointers. It shall be used for cache of pre-built gpu
binary code and
metadata cache of apache arrow files.
However, the assertion check above requires extension to set a fake node-tag
to avoid backend crash. Right now, it is harmless to set T_AllocSetContext, but
feel a bit bad.

Interesting. Does that mean the hared memory contexts are part of the
same hierarchy as "normal" contexts? That would be a bit confusing, I
think.

If this shared memory context is a child of normal context, likely, it should be
reset or deleted as usual. However, if this shared memory context performs
as a parent of normal context, it makes a problem when different process tries
to delete this context, because its child (normal context) exists at the creator
process only. So, it needs to be used carefully.

Yeah, handling life cycle of a mix of those contexts may be quite tricky.

But my concern was a bit more general - is it a good idea to hide the
nature of the memory context behind the same API. If you call palloc()
shouldn't you really know whether you're allocating the stuff in regular
or shared memory context?

Maybe it makes perfect sense, but my initial impression is that those
seem rather different, so maybe we should keep them separate (in
separate hierarchies or something). But I admit I don't have much
experience with use cases that would require such shmem contexts.

Yeah, as you mentioned, we have no way to distinguish whether a particular
memory chunk is private memory or shared memory, right now.
It is the responsibility of software developers, and I assume the shared-
memory chunks are applied on a limited usages where it has a certain reason
why it should be shared.
On the other hand, it is the same situation even if private memory.
We should pay attention to the memory context to allocate a memory chunk from.
For example, state object to be valid during query execution must be allocated
from estate->es_query_cxt. If someone allocates it from CurrentMemoryContext,
then implicitly released, we shall consider it is a bug.

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

2020年1月29日(水) 2:18 Robert Haas <robertmhaas@gmail.com>:

On Tue, Jan 28, 2020 at 11:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I strongly object to having the subtype field be just "char".
I want it to be declared "MemoryContextType", so that gdb will
still be telling me explicitly what struct type this really is.
I realize that you probably did that for alignment reasons, but
maybe we could shave the magic number down to 2 bytes, and then
rearrange the field order? Or just not sweat so much about
wasting a longword here. Having those bools up at the front
is pretty ugly anyway.

I kind of dislike having the magic number not be the first thing in
the struct on aesthetic grounds, and possibly on the grounds that
somebody might be examining the initial bytes manually to try to
figure out what they've got, and having the magic number there makes
it easier. Regarding space consumption, I guess this structure is
already over 64 bytes and not close to 128 bytes, so adding another 8
bytes probably isn't very meaningful, but I don't love it. One thing
that concerns me a bit is that if somebody adds their own type of
memory context, then MemoryContextType will have a value that is not
actually in the enum. If compilers are optimizing the code on the
assumption that this cannot occur, do we need to worry about undefined
behavior?

Actually, I have what I think is a better idea. I notice that the
"type" field is actually completely unused. We initialize it, and then
nothing in the code ever checks it or relies on it for any purpose.
So, we could have a bug in the code that initializes that field with
the wrong value, for a new context type or in general, and only a
developer with a debugger would ever notice. 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.

Here's a v2 that approaches the problem that way.

How about to have "const char *name" in MemoryContextMethods?
It is more human readable for debugging, than raw function pointers.
We already have similar field to identify the methods at CustomScanMethods.
(it is also used for EXPLAIN, not only debugging...)

I love the idea to identify the memory-context type with single identifiers
rather than two. If we would have sub-field Id and memory-context methods
separately, it probably needs Assert() to check the consistency of
them, isn't it?

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

2020年1月29日(水) 4:27 Thomas Munro <thomas.munro@gmail.com>:

On Tue, Jan 28, 2020 at 2:55 PM Kohei KaiGai <kaigai@heterodb.com> wrote:

I noticed MemoryContextIsValid() called by various kinds of memory context
routines checks its node-tag as follows:

#define MemoryContextIsValid(context) \
((context) != NULL && \
(IsA((context), AllocSetContext) || \
IsA((context), SlabContext) || \
IsA((context), GenerationContext)))

It allows only "known" memory context methods, even though the memory context
mechanism enables to implement custom memory allocator by extensions.
Here is a node tag nobody used: T_MemoryContext.
It looks to me T_MemoryContext is a neutral naming for custom memory context,
and here is no reason why memory context functions prevents custom methods.

https://github.com/heterodb/pg-strom/blob/master/src/shmbuf.c#L1243
I recently implemented a custom memory context for shared memory allocation
with portable pointers. It shall be used for cache of pre-built gpu
binary code and
metadata cache of apache arrow files.
However, the assertion check above requires extension to set a fake node-tag
to avoid backend crash. Right now, it is harmless to set T_AllocSetContext, but
feel a bit bad.

FWIW the code in https://commitfest.postgresql.org/26/2325/ ran into
exactly the same problem while making nearly exactly the same kind of
thing (namely, a MemoryContext backed by space in the main shm area,
in this case reusing the dsa.c allocator).

Thanks, I had looked at "Shared Memory Context" title on pgsql-hackers a few
months ago, however, missed the thread right now.

The main point of the differences from this approach is portability of pointers
to shared memory chunks. (If I understand correctly)
PG-Strom preserves logical address space, but no physical pages, on startup
time, then maps shared memory segment on the fixed address on demand.
So, pointers are portable to all the backend processes, thus, suitable to build
tree structure on shared memory also.

This article below introduces how our "shared memory context" works.
It is originally written in Japanese, and Google translate may
generate unnatural
English. However, its figures probably explain how it works.
https://translate.google.co.jp/translate?hl=ja&amp;sl=auto&amp;tl=en&amp;u=http%3A%2F%2Fkaigai.hatenablog.com%2Fentry%2F2016%2F06%2F19%2F095127

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

On Wed, Jan 29, 2020 at 2:49 PM Kohei KaiGai <kaigai@heterodb.com> wrote:

2020年1月29日(水) 4:27 Thomas Munro <thomas.munro@gmail.com>:

On Tue, Jan 28, 2020 at 2:55 PM Kohei KaiGai <kaigai@heterodb.com> wrote:
FWIW the code in https://commitfest.postgresql.org/26/2325/ ran into
exactly the same problem while making nearly exactly the same kind of
thing (namely, a MemoryContext backed by space in the main shm area,
in this case reusing the dsa.c allocator).

Thanks, I had looked at "Shared Memory Context" title on pgsql-hackers a few
months ago, however, missed the thread right now.

The main point of the differences from this approach is portability of pointers
to shared memory chunks. (If I understand correctly)
PG-Strom preserves logical address space, but no physical pages, on startup
time, then maps shared memory segment on the fixed address on demand.
So, pointers are portable to all the backend processes, thus, suitable to build
tree structure on shared memory also.

Very interesting. PostgreSQL's DSM segments could potentially have
been implemented that way (whereas today they are not mapped with
MAP_FIXED), but I suppose people were worried about portability
problems and ASLR. The Windows port struggles with that stuff.

Actually the WIP code in that patch reserves a chunk of space up front
in the postmaster, and then puts a DSA allocator inside it. Normally,
DSA allocators create/destroy new DSM segments on demand and deal with
the address portability stuff through a lot of extra work (this
probably makes Parallel Hash Join slightly slower than it should be),
but as a special case a DSA allocator can be created in preexisting
memory and then not allowed to grow. In exchange for accepting a
fixed space, you get normal shareable pointers. This means that you
can use the resulting weird MemoryContext for stuff like building
query plans that can be used by other processes, but when you run out
of memory, allocations begin to fail. That WIP code is experimenting
with caches that can tolerate running out of memory (or at least that
is the intention), so a fixed sized space is OK for that.

This article below introduces how our "shared memory context" works.
It is originally written in Japanese, and Google translate may
generate unnatural
English. However, its figures probably explain how it works.
https://translate.google.co.jp/translate?hl=ja&amp;sl=auto&amp;tl=en&amp;u=http%3A%2F%2Fkaigai.hatenablog.com%2Fentry%2F2016%2F06%2F19%2F095127

Thanks!

On Tue, Jan 28, 2020 at 10:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

FWIW, I noticed that GDB becomes much better at this when you add "set
print symbol on" to your .gdbinit dot file about a year ago. In theory
you shouldn't need to do that to print the symbol that a function
pointer points to, I think. At least that's what the documentation
says. But in practice this seems to help a lot.

I don't recall figuring out a reason for this. Could have been due to
GDB being fussier about the declared type of a pointer than it needs
to be, or something along those lines.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

On Tue, Jan 28, 2020 at 10:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

FWIW, I noticed that GDB becomes much better at this when you add "set
print symbol on" to your .gdbinit dot file about a year ago.

Interesting. But I bet there are platform and version dependencies
in the mix, too. I'd still not wish to rely on this for debugging.

regards, tom lane

On Tue, Jan 28, 2020 at 6:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

FWIW, I noticed that GDB becomes much better at this when you add "set
print symbol on" to your .gdbinit dot file about a year ago.

Interesting. But I bet there are platform and version dependencies
in the mix, too. I'd still not wish to rely on this for debugging.

I agree that there are a lot of moving pieces here. I wouldn't like to
have to rely on this working myself.

--
Peter Geoghegan

2020年1月29日(水) 11:06 Thomas Munro <thomas.munro@gmail.com>:

On Wed, Jan 29, 2020 at 2:49 PM Kohei KaiGai <kaigai@heterodb.com> wrote:

2020年1月29日(水) 4:27 Thomas Munro <thomas.munro@gmail.com>:

On Tue, Jan 28, 2020 at 2:55 PM Kohei KaiGai <kaigai@heterodb.com> wrote:
FWIW the code in https://commitfest.postgresql.org/26/2325/ ran into
exactly the same problem while making nearly exactly the same kind of
thing (namely, a MemoryContext backed by space in the main shm area,
in this case reusing the dsa.c allocator).

Thanks, I had looked at "Shared Memory Context" title on pgsql-hackers a few
months ago, however, missed the thread right now.

The main point of the differences from this approach is portability of pointers
to shared memory chunks. (If I understand correctly)
PG-Strom preserves logical address space, but no physical pages, on startup
time, then maps shared memory segment on the fixed address on demand.
So, pointers are portable to all the backend processes, thus, suitable to build
tree structure on shared memory also.

Very interesting. PostgreSQL's DSM segments could potentially have
been implemented that way (whereas today they are not mapped with
MAP_FIXED), but I suppose people were worried about portability
problems and ASLR. The Windows port struggles with that stuff.

Yes. I'm not certain whether Windows can support equivalen behavior
to mmap(..., PROT_NONE) and SIGSEGV/SIGBUS handling.
It is also a reason why PG-Strom (that is only for Linux) wants to have
own shared memory management logic, at least, right now.

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

Hi,

On 2020-01-28 11:10:47 -0500, Robert Haas wrote:

I generally like this idea, but I'd like to propose that we instead
replace the NodeTag with a 4-byte magic number. I was studying how
feasible it would be to make memory contexts available in frontend
code, and it doesn't look all that bad, but one of the downsides is
that nodes/memnodes.h includes nodes/nodes.h, and I think it would not
be a good idea to make frontend code depend on nodes/nodes.h, which
seems like it's really a backend-only piece of infrastructure. Using a
magic number would allow us to avoid that, and it doesn't really cost
anything, because the memory context nodes really don't participate in
any of that infrastructure anyway.

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.

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.

Greetings,

Andres Freund