Re: Add memory context type to pg_backend_memory_contexts view

> On May 30, 2024, at 5:36 PM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Fri, 31 May 2024 at 07:21, David Christensen <david+pg(at)pgguru(dot)net> wrote:
>> Giving this a once-through, this seems straightforward and useful. I
>> have a slight preference for keeping "name" the first field in the
>> view and moving "type" to the second, but that's minor.
>
> Not having it first make sense, but I don't think putting it between
> name and ident is a good idea. I think name and ident belong next to
> each other. parent likely should come after those since that also
> relates to the name.
>
> How about putting it after "parent"?

That works for me. I skimmed the new patch and it seems fine but on my phone so didn’t do any build tests.

>> Just confirming that the allocator types are not extensible without a
>> recompile, since it's using a specific node tag to switch on, so there
>> are no concerns with not properly displaying the output of something
>> else.
>
> They're not extensible.

Good to confirm.

>> The "????" text placeholder might be more appropriate as "<unknown>",
>> or perhaps stronger, include a WARNING in the logs, since an unknown
>> tag at this point would be an indication of some sort of memory
>> corruption.
>
> This follows what we do in other places. If you look at explain.c,
> you'll see lots of "???"s.
>
> I think if you're worried about corrupted memory, then garbled output
> in pg_get_backend_memory_contexts wouldn't be that high on the list of
> concerns.

Heh, indeed. +1 for precedent.

>> Since there are only four possible values, I think there would be
>> utility in including them in the docs for this field.
>
> I'm not sure about this. We do try not to expose too much internal
> detail in the docs. I don't know all the reasons for that, but at
> least one reason is that it's easy for things to get outdated as code
> evolves. I'm also unsure how much value there is in listing 4 possible
> values unless we were to document the meaning of each of those values,
> and doing that puts us even further down the path of detailing
> Postgres internals in the documents. I don't think that's a
> maintenance burden that's often worth the trouble.

I can see that and it’s consistent with what we do, just was thinking as a user that that may be useful, but if you’re using this view you likely already know what it means.

>> I also think it
>> would be useful to have some sort of comments at least in mmgr/README
>> to indicate that if a new type of allocator is introduced that you
>> will also need to add the node to the function for this type, since
>> it's not an automatic conversion.
>
> I don't think it's sustainable to do this. If we have to maintain
> documentation that lists everything you must do in order to add some
> new node types then I feel it's just going to get outdated as soon as
> someone adds something new that needs to be done. I'm only one
> developer, but for me, I'd not even bother looking there if I was
> planning to add a new memory context type.
>
> What I would be doing is searching the entire code base for where
> special handling is done for the existing types and ensuring I
> consider if I need to include a case for the new node type. In this
> case, I'd probably choose to search for "T_AllocSetContext", and I'd
> quickly land on PutMemoryContextsStatsTupleStore() and update it. This
> method doesn't get outdated, provided you do "git pull" occasionally.

Fair.

>> (For that matter, instead of
>> switching on node type and outputting a given string, is there a
>> generic function that could just give us the string value for node
>> type so we don't need to teach anything else about it anyway?)
>
> There isn't. nodeToString() does take some node types as inputs and
> serialise those to something JSON-like, but that includes serialising
> each field of the node type too. The memory context types are not
> handled by those functions. I think it's fine to copy what's done in
> explain.c. "git grep \"???\" -- *.c | wc -l" gives me 31 occurrences,
> so I'm not doing anything new.
>
> I've attached an updated patch which changes the position of the new
> column in the view.

+1