> On May 30, 2024, at 5:36 PM, David Rowley <dgrowle...@gmail.com> wrote: > > On Fri, 31 May 2024 at 07:21, David Christensen <david...@pgguru.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
