Thread

Re: Enhancing Memory Context Statistics Reporting

Daniel Gustafsson <daniel@yesql.se> — 2025-12-08T09:43:18Z

> On 28 Nov 2025, at 10:22, Rahila Syed <rahilasyed90@gmail.com> wrote:
> 
> Hi,
> 
> I'm attaching the updated patches, which primarily include cleanup and have been rebased
> following the CFbot report.  

Thanks for the patch, below are a few comments and suggestions.  As I was
reviewing I tweaked the below and have attached the comments as changes in
0003.

== in func-admin.sgml
+        <function>pg_get_process_memory_contexts</function> ( <parameter>pid</parameter> <type>integer</type>, <parameter>summary</parameter> <type>boolean</type> )
We recently simplified the UI by removing the timeout, and the more I think
about it the more I am convinced that there is more simplification to be had.
The most likely usage pattern, IMO, will be to get all the contexts and not the
summary, so we can make the summary parameter DEFAULT to false.  This allows
most uses to just pass the pid, without complicating the code at all.

== in mcxtfuncs.c
+/* Size of dshash key */
+#define CLIENT_KEY_SIZE 32
That's still pretty generous isn't it?  We are printing a uint32 into it so the
highest number it can reach is ~4 billion (which while the upper limit, is
quite theoretic in this case).  10 + 1 bytes should suffice to store right?

- * MemoryContextId
+ * MemoryStatsContextId
Sorry, but I still don't agree with this rename and I think we should skip it,
if only to avoid changes to existing parts of the code.

+    * Entry has been deleted due to client process exit. Make sure that the
+    * client always deletes the entry after taking required lock or this
+    * function may end up writing to unallocated memory.
Can you explain this a bit further, I'm not sure I get it.  The code goes on to
release a lock immediately and then destroys the hash.  Who is responsible for
destroying the entry?


== In system-views.sql
+REVOKE EXECUTE ON FUNCTION
+   pg_get_process_memory_contexts(integer, boolean) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION
+   pg_get_process_memory_contexts(integer, boolean) TO pg_read_all_stats;
This is not a view, and the functions aren't used to drive a view, so these
should not be defined here.  The above mentioned change to add DEFAULT handling
to the summary parameter fixes this in the attached.

== In ProcessGetMemoryContextInterrupt()
I'm not a fan of having to exit's from the function doing duplicative cleanups,
in the attached I've wrapped them in a conditional to just have one exit path.
What do you think about that?


== In PublishMemoryContext()
+   end_memorycontext_reporting(entry, oldcontext, context_id_lookup);
Looking at this more I don't really like that resetting the memory context is
done via a separate function, when that function must be called from the exact
right place to ensure CurrentMemoryContext is what it thinks it is.  It's all a
bit too magic.  Since this is only called in 3 places I would prefer to inline
the code in PublishMemoryContext().

== In PublishMemoryContext()
+   const char *ident = context->ident;
+   const char *name = context->name;
ident and name are defined as const, but they are later assigned to after the
initial assignment. I think we need to unconstify these.


== In PublishMemoryContext()
+       if (strlen(name) >= MEMORY_CONTEXT_NAME_SHMEM_SIZE)
We already have namelen which is set to exactly strlen(name), so let's reuse
that for readability.


== In PublishMemoryContext()
+   /* Allocate DSA memory for storing path information */
This comment is no longer accurate is it?  The DSA has already been allocated
at this point.


== In memutils.h
+#include "utils/dsa.h"
This is not needed.

I also did some smaller comment rewording and reflowing, some smaller cleanups
and a fresh pgindent/pgperltidy run. The attached 0003 contains the above.

--
Daniel Gustafsson