Subject: Re: Enhancing Memory Context Statistics Reporting

From: torikoshia <[email-masked]>
Date: Mon, 16 Dec 2024 21:51:53 +0900

Hi,

Thanks for updating the patch and here are some comments:

'path' column of pg_get_process_memory_contexts() begins with 0, but 
that column of pg_backend_memory_contexts view begins with 1:

   =# select path FROM pg_get_process_memory_contexts('20271', false);
   path
   -------
    {0}
    {0,1}
    {0,2}
    ..

=# select path from pg_backend_memory_contexts;
    path
   -------
    {1}
    {1,2}
    {1,3}
    ..asdf asdf

Would it be better to begin with 1 to make them consistent?

pg_log_backend_memory_contexts() does not allow non-superusers to 
execute by default since it can peek at other session information.
pg_get_process_memory_contexts() does not have this restriction, but 
wouldn't it be necessary?

When the target pid is the local backend, the HINT suggests using 
pg_get_backend_memory_contexts(), but this function is not described in 
the manual.
How about suggesting pg_backend_memory_contexts view instead?

   =# select pg_get_process_memory_contexts('27041', false);
   WARNING:  cannot return statistics for local backend
   HINT:  Use pg_get_backend_memory_contexts instead

There are no explanations about 'num_agg_contexts', but I thought the 
explanation like below would be useful.

> I have added this information as a column named "num_agg_contexts", 
> which indicates
> the number of contexts whose statistics have been aggregated/added for 
> a particular output.

git apply caused some warnings:

$ git apply 
v7-Function-to-report-memory-context-stats-of-any-backe.patch
v7-Function-to-report-memory-context-stats-of-any-backe.patch:71: space 
before tab in indent.
         Requests to return the memory contexts of the backend with the
v7-Function-to-report-memory-context-stats-of-any-backe.patch:72: space 
before tab in indent.
         specified process ID.  This function can send the request to
v7-Function-to-report-memory-context-stats-of-any-backe.patch:ldmv: 
space before tab in indent.
         both the backends and auxiliary processes. After receiving the 
memory
v7-Function-to-report-memory-context-stats-of-any-backe.patch:74: space 
before tab in indent.
         contexts from the process, it returns the result as one row per
v7-Function-to-report-memory-context-stats-of-any-backe.patch:75: space 
before tab in indent.
         context. When get_summary is true, memory contexts at level 0

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.

---

From: Rahila Syed <[email-masked]>
Date: Mon, 6 Jan 2025 18:46:30 +0530

Hi Torikoshia,

Thank you for the review.

> Makes sense, fixed in the attached patch.

> Yes. I added the restriction to only allow super users and
users with pg_read_all_stats privileges to query the memory context
statistics of another process.

> Ok. I added an explanation of this column in the documentation.

> Thank you for reporting. They should be gone now.

PFA the patch with above updates.

Thank you,
Rahila Syed

---

From: Fujii Masao <[email-masked]>
Date: Tue, 7 Jan 2025 02:04:20 +0900

> PFA the patch with above updates.

Thanks for updating the patch! I like this feature.

I tested this feature and encountered two issues:

Issue 1: Error with pg_get_process_memory_contexts()
When I used pg_get_process_memory_contexts() on the PID of a backend process
that had just caused an error but hadn’t rolled back yet,
the following error occurred:

   Session 1 (PID=70011):
   =# begin;
   =# select 1/0;
   ERROR:  division by zero
   
   Session 2:
   =# select * from pg_get_process_memory_contexts(70011, false);
   
   Session 1 terminated with:
   ERROR:  ResourceOwnerEnlarge called after release started
   FATAL:  terminating connection because protocol synchronization was lost

Issue 2: Segmentation Fault
When I ran pg_get_process_memory_contexts() every 0.1 seconds using
\watch command while running "make -j 4 installcheck-world",
I encountered a segmentation fault:

   LOG:  client backend (PID 97975) was terminated by signal 11: Segmentation fault: 11
   DETAIL:  Failed process was running: select infinite_recurse();
   LOG:  terminating any other active server processes

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 6 Jan 2025 22:02:28 +0100

Hi Rahila,

Thanks for the updated and rebased patch. I've tried the pgbench test
again, to see if it gets stuck somewhere, and I'm observing this on a
new / idle cluster:

$ pgbench -n -f test.sql -P 1 test -T 60
pgbench (18devel)
progress: 1.0 s, 1647.9 tps, lat 0.604 ms stddev 0.438, 0 failed
progress: 2.0 s, 1374.3 tps, lat 0.727 ms stddev 0.386, 0 failed
progress: 3.0 s, 1514.4 tps, lat 0.661 ms stddev 0.330, 0 failed
progress: 4.0 s, 1563.4 tps, lat 0.639 ms stddev 0.212, 0 failed
progress: 5.0 s, 1665.0 tps, lat 0.600 ms stddev 0.177, 0 failed
progress: 6.0 s, 1538.0 tps, lat 0.650 ms stddev 0.192, 0 failed
progress: 7.0 s, 1491.4 tps, lat 0.670 ms stddev 0.261, 0 failed
progress: 8.0 s, 1539.5 tps, lat 0.649 ms stddev 0.443, 0 failed
progress: 9.0 s, 1517.0 tps, lat 0.659 ms stddev 0.167, 0 failed
progress: 10.0 s, 1594.0 tps, lat 0.627 ms stddev 0.227, 0 failed
progress: 11.0 s, 28.0 tps, lat 0.705 ms stddev 0.277, 0 failed
progress: 12.0 s, 0.0 tps, lat 0.000 ms stddev 0.000, 0 failed
progress: 13.0 s, 0.0 tps, lat 0.000 ms stddev 0.000, 0 failed
progress: 14.0 s, 0.0 tps, lat 0.000 ms stddev 0.000, 0 failed
progress: 15.0 s, 0.0 tps, lat 0.000 ms stddev 0.000, 0 failed
progress: 16.0 s, 1480.6 tps, lat 4.043 ms stddev 130.113, 0 failed
progress: 17.0 s, 1524.9 tps, lat 0.655 ms stddev 0.286, 0 failed
progress: 18.0 s, 1246.0 tps, lat 0.802 ms stddev 0.330, 0 failed
progress: 19.0 s, 1383.1 tps, lat 0.722 ms stddev 0.934, 0 failed
progress: 20.0 s, 1432.7 tps, lat 0.698 ms stddev 0.199, 0 failed
...

There's always a period of 10-15 seconds when everything seems to be
working fine, and then a couple seconds when it gets stuck, with the usual

  LOG:  Wait for 69454 process to publish stats timed out, trying again

The PIDs I've seen were for checkpointer, autovacuum launcher, ... all
of that are processes that should be handling the signal, so how come it
gets stuck every now and then? The system is entirely idle, there's no
contention for the shmem stuff, etc. Could it be forgetting about the
signal in some cases, or something like that?

The test.sql is super simple:

  SELECT * FROM pg_get_process_memory_contexts(
    (SELECT pid FROM pg_stat_activity
      WHERE pid != pg_backend_pid()
      ORDER BY random() LIMIT 1)
    , false);

Aside from this, I went through the patch to do a regular review, so
here's the main comments in somewhat random order:

1) The SGML docs talk about "contexts at level" but I don't think that's
defined/explained anywhere, there are different ways to assign levels in
a tree-like structure, so it's unclear if levels are assigned from the
top or bottom.

2) volatile sig_atomic_t PublishMemoryContextPending = false;

I'd move this right after LogMemoryContextPending (to match the other
places that add new stuff).

3) typedef enum PrintDetails

I suppose this should have some comments, explaining what the typedef is
for. Also, "details" sounds pretty generic, perhaps "destination" or
maybe "target" would be better?

4) The memcpy here seems unnecessary - the string is going to be static
in the binary, no need to copy it. In which case the whole switch is
going to be the same as in PutMemoryContextsStatsTupleStore, so maybe
move that into a separate function?

+    switch (context->type)
+    {
+        case T_AllocSetContext:
+            type = "AllocSet";
+            strncpy(memctx_info[curr_id].type, type, strlen(type));
+            break;
+        case T_GenerationContext:
+            type = "Generation";
+            strncpy(memctx_info[curr_id].type, type, strlen(type));
+            break;
+        case T_SlabContext:
+            type = "Slab";
+            strncpy(memctx_info[curr_id].type, type, strlen(type));
+            break;
+        case T_BumpContext:
+            type = "Bump";
+            strncpy(memctx_info[curr_id].type, type, strlen(type));
+            break;
+        default:
+            type = "???";
+            strncpy(memctx_info[curr_id].type, type, strlen(type));
+            break;
+    }

5) The comment about hash table in ProcessGetMemoryContextInterrupt
seems pretty far from hash_create(), so maybe move it.

6) ProcessGetMemoryContextInterrupt seems pretty long / complex, with
multiple nested loops, it'd be good to split it into smaller parts that
are easier to understand.

7) I'm not sure if/why we need to move MemoryContextId to memutils.h.

8) The new stuff in memutils.h is added to the wrong place, into a
section labeled "Memory-context-type-specific functions" (which it
certainly is not)

9) autovacuum.c adds the ProcessGetMemoryContextInterrupt() call after
ProcessCatchupInterrupt() - that's not wrong, but I'd move it right
after ProcessLogMemoryContextInterrupt(), just like everywhere else.

10) The pg_get_process_memory_contexts comment says:

  Signal a backend or an auxiliary process to send its ...

But this is not just about the signal, it also waits for the results and
produces the result set.

11) pg_get_process_memory_contexts - Wouldn't it be better to move the
InitMaterializedSRF() call until after the privilege check, etc.?

12) The pg_get_process_memory_contexts comment should explain why it's
superuser-only function. Presumably it has similar DoS risks as the
other functions, because if not why would we have the restriction?

13) I reworded and expanded the pg_get_process_memory_contexts comment a
bit, and re-wrapped it too. But I think it also needs to explain how it
communicates with the other process (sending signal, sending data
through a DSA, ...). And also how the timeouts work.

14) I'm a bit confused about the DSA allocations (but I also haven't
worked with DSA very much, so maybe it's fine). Presumably the 16MB is
upper limit, we won't use that all the time. We allocate 1MB, but allow
it to grow up to 16MB, correct? 16MB seems like a lot, certainly enough
for this purpose - if it's not, I don't think we can come up with a
better limit.

15) In any case, I don't think the 16 should be hardcoded as a magic
constant in multiple places. That's bound to be error-prone.

16) I've reformatted / reindented / wrapped the code in various places,
to make it easier to read and more consistent with the nearby code. I
also added a bunch of comments explaining what the block of code is
meant to do (I mean, what it aims to do).

16) A comment in pg_get_process_memory_contexts says:

  Pin the mapping so that it doesn't throw a warning

That doesn't seem very useful. It's not clear what kind of warning this
hides, but more importantly - we're not doing stuff to hide some sort of
warning, we do it to prevent what the warning is about.

17) pg_get_process_memory_contexts has a bunch of error cases, where we
need to detach the DSA and return NULL. Would be better to do a label
with a goto, I think.

18) I think pg_get_process_memory_contexts will have issues if this
happens in the first loop:

    if ((memCtxState[procNumber].proc_id == pid) &&
        DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer))
        break;

Because then we end up with memctx_info pointing to garbage after the
loop. I don't know how hard is to hit this, I guess it can happen in
many processes calling pg_get_process_memory_contexts?

19) Minor comment and formatting of MemCtxShmemSize / MemCtxShmemInit.

20) MemoryContextInfo etc. need to be added to typedefs.list, so that
pgindent can do the right thing.

21) I think ProcessGetMemoryContextInterrupt has a bug because it uses
get_summary before reading it from the shmem.

Attached are two patches - 0001 is the original patch, 0002 has most of
my review comments (mentioned above), and a couple additional changes to
comments/formatting, etc. Those are suggestions rather than issues.

regards

-- 
Tomas Vondra
From be12803e8dbc671595f7945693b3f70abb2b8745 Mon Sep 17 00:00:00 2001
From: Rahila Syed <[email-masked]>
Date: Sun, 15 Sep 2024 17:56:06 +0530
Subject: [PATCH vtomas 1/2] Function to report memory context stats of any
 backend

This function sends a signal to a backend to publish
statistics of all its memory contexts. Signal handler
sets a flag, which causes the relevant backend to copy its
MemoryContextStats to a DSA, as part
of next CHECK_FOR_INTERRUPTS().
It there are more that 16MB worth of statistics, the
remaining statistics are copied as a cumulative
total of the remaining contexts.
Once its done, it signals the client backend using
a condition variable. The client backend
then wakes up, reads the shared memory and
returns these values in the form of set of records,
one for each memory context, to the user, followed
by a cumulative total of the remaining contexts,
if any.

Each backend and auxiliary process has its own slot
for reporting the stats. There is an array of such
memory slots of size MaxBackends+NumofAuxiliary
processes in fixed shared memory. Each of these slots point
to a DSA, which contains the stats to be shared by the
corresponding process.
Each slot has its own LW lock and condition variable for
synchronization and communication between the
publishing process and the client backend.
---
 doc/src/sgml/func.sgml                        |  26 ++
 src/backend/postmaster/autovacuum.c           |   4 +
 src/backend/postmaster/checkpointer.c         |   4 +
 src/backend/postmaster/interrupt.c            |   4 +
 src/backend/postmaster/pgarch.c               |   4 +
 src/backend/postmaster/startup.c              |   4 +
 src/backend/postmaster/walsummarizer.c        |   4 +
 src/backend/storage/ipc/ipci.c                |   2 +
 src/backend/storage/ipc/procsignal.c          |   3 +
 src/backend/tcop/postgres.c                   |   3 +
 .../utils/activity/wait_event_names.txt       |   1 +
 src/backend/utils/adt/mcxtfuncs.c             | 274 ++++++++++-
 src/backend/utils/init/globals.c              |   1 +
 src/backend/utils/mmgr/mcxt.c                 | 424 +++++++++++++++++-
 src/include/catalog/pg_proc.dat               |  10 +
 src/include/miscadmin.h                       |   1 +
 src/include/storage/procsignal.h              |   1 +
 src/include/utils/memutils.h                  |  51 +++
 src/test/regress/expected/sysviews.out        |  12 +
 src/test/regress/sql/sysviews.sql             |  12 +
 20 files changed, 822 insertions(+), 23 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 47370e581ae..5d0399508ea 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28358,6 +28358,32 @@ acl      | {postgres=arwdDxtm/postgres,foo=r/postgres}
        </para></entry>
       </row>
 
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_get_process_memory_contexts</primary>
+        </indexterm>
+        <function>pg_get_process_memory_contexts</function> ( <parameter>pid</parameter> <type>integer</type>, <parameter>get_summary</parameter> <type>boolean</type> )
+        <returnvalue>setof record</returnvalue>
+       </para>
+       <para>
+        This function handles requests to display the memory contexts of a
+        PostgreSQL process with the specified process ID (PID). It takes two
+        arguments: PID and a boolean, get_summary. The function can send requests
+        to both backend and auxiliary processes.
+
+        After receiving memory context statistics from the target process, it
+        returns the results as one row per context. The num_agg_contexts column
+        indicates the number of contexts aggregated in the displayed statistics.
+
+        When get_summary is set to true, memory context statistics at levels 1 and 2,
+        are displayed with each level 2 context showing a cumulative total of all
+        its child contexts.
+        When get_summary is set to false, the num_agg_contexts value is 1, indicating
+        that individual statistics are being displayed.
+       </para></entry>
+      </row>
+
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 3f826532b88..eb4c98a17bc 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -768,6 +768,10 @@ HandleAutoVacLauncherInterrupts(void)
 
 	/* Process sinval catchup interrupts that happened while sleeping */
 	ProcessCatchupInterrupt();
+
+	/* Publish memory contexts of this process */
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
 }
 
 /*
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 9bfd0fd665c..ee8360ad6fa 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -616,6 +616,10 @@ HandleCheckpointerInterrupts(void)
 	/* Perform logging of memory contexts of this process */
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
+
+	/* Publish memory contexts of this process */
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
 }
 
 /*
diff --git a/src/backend/postmaster/interrupt.c b/src/backend/postmaster/interrupt.c
index be69e4c7136..9481a5cd241 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -48,6 +48,10 @@ HandleMainLoopInterrupts(void)
 	/* Perform logging of memory contexts of this process */
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
+
+	/* Publish memory contexts of this process */
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
 }
 
 /*
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c
index 12ee815a626..cd1ecb6b93d 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -865,6 +865,10 @@ HandlePgArchInterrupts(void)
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
 
+	/* Publish memory contexts of this process */
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
+
 	if (ConfigReloadPending)
 	{
 		char	   *archiveLib = pstrdup(XLogArchiveLibrary);
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index 59d213031b3..d670954c4e9 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -192,6 +192,10 @@ HandleStartupProcInterrupts(void)
 	/* Perform logging of memory contexts of this process */
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
+
+	/* Publish memory contexts of this process */
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
 }

diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index ffbf0439358..b1a5e86a85c 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -876,6 +876,10 @@ HandleWalSummarizerInterrupts(void)
 	/* Perform logging of memory contexts of this process */
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
+
+	/* Publish memory contexts of this process */
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
 }
 
 /*
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 174eed70367..4a70eabf7f3 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -50,6 +50,7 @@
 #include "storage/sinvaladt.h"
 #include "utils/guc.h"
 #include "utils/injection_point.h"
+#include "utils/memutils.h"
 
 /* GUCs */
 int			shared_memory_type = DEFAULT_SHARED_MEMORY_TYPE;
@@ -340,6 +341,7 @@ CreateOrAttachShmemStructs(void)
 	StatsShmemInit();
 	WaitEventCustomShmemInit();
 	InjectionPointShmemInit();
+	MemCtxShmemInit();
 }
 
 /*
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 7401b6e625e..e425b9eeb03 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -688,6 +688,9 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
 	if (CheckProcSignal(PROCSIG_LOG_MEMORY_CONTEXT))
 		HandleLogMemoryContextInterrupt();
 
+	if (CheckProcSignal(PROCSIG_GET_MEMORY_CONTEXT))
+		HandleGetMemoryContextInterrupt();
+
 	if (CheckProcSignal(PROCSIG_PARALLEL_APPLY_MESSAGE))
 		HandleParallelApplyMessageInterrupt();
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c01cff9d650..0eae9be122b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3497,6 +3497,9 @@ ProcessInterrupts(void)
 	if (LogMemoryContextPending)
 		ProcessLogMemoryContextInterrupt();
 
+	if (PublishMemoryContextPending)
+		ProcessGetMemoryContextInterrupt();
+
 	if (ParallelApplyMessagePending)
 		HandleParallelApplyMessages();
 }
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 0b53cba807d..68a17699675 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -158,6 +158,7 @@ WAL_RECEIVER_EXIT	"Waiting for the WAL receiver to exit."
 WAL_RECEIVER_WAIT_START	"Waiting for startup process to send initial data for streaming replication."
 WAL_SUMMARY_READY	"Waiting for a new WAL summary to be generated."
 XACT_GROUP_UPDATE	"Waiting for the group leader to update transaction status at transaction end."
+MEM_CTX_PUBLISH	"Waiting for backend to publish memory information."
 
 ABI_compatibility:
 
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 396c2f223b4..c067cdf8709 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -17,28 +17,25 @@
 
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
+#include "miscadmin.h"
+#include "access/twophase.h"
+#include "catalog/pg_authid_d.h"
+#include "nodes/pg_list.h"
 #include "storage/proc.h"
 #include "storage/procarray.h"
+#include "utils/acl.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/hsearch.h"
+#include "utils/memutils.h"
+#include "utils/wait_event_types.h"
 
 /* ----------
  * The max bytes for showing identifiers of MemoryContext.
  * ----------
  */
-#define MEMORY_CONTEXT_IDENT_DISPLAY_SIZE	1024
 
-/*
- * MemoryContextId
- *		Used for storage of transient identifiers for
- *		pg_get_backend_memory_contexts.
- */
-typedef struct MemoryContextId
-{
-	MemoryContext context;
-	int			context_id;
-}			MemoryContextId;
+struct MemoryContextState *memCtxState = NULL;
 
 /*
  * int_list_to_array
@@ -71,7 +68,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
 								 TupleDesc tupdesc, MemoryContext context,
 								 HTAB *context_id_lookup)
 {
-#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	10
+#define PG_GET_BACKEND_MEMORY_CONTEXTS_COLS	11
 
 	Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
 	bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
@@ -305,3 +302,256 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
 
 	PG_RETURN_BOOL(true);
 }
+
+/*
+ * pg_get_process_memory_contexts
+ *		Signal a backend or an auxiliary process to send its memory contexts.
+ *
+ * On receipt of this signal, a backend or an auxiliary process sets the flag
+ * in the signal handler, which causes the next CHECK_FOR_INTERRUPTS()
+ * or process-specific interrupt handler to copy the memory context statistics
+ * in a dynamic shared memory space. The statistics for contexts that do not fit in
+ * shared memory area are stored as a cumulative total of those contexts,
+ * at the end in the dynamic shared memory.
+ * Wait for the backend to send signal on the condition variable after
+ * writing statistics to a shared memory.
+ * Once condition variable comes out of sleep, check if the required
+ * backends statistics are available to read and display.
+ */
+Datum
+pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
+{
+	int			pid = PG_GETARG_INT32(0);
+	bool		get_summary = PG_GETARG_BOOL(1);
+	PGPROC	   *proc;
+	ProcNumber	procNumber = INVALID_PROC_NUMBER;
+	int			i;
+	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+	dsa_area   *area;
+	dsa_handle	handle;
+	MemoryContextInfo *memctx_info;
+	MemoryContext oldContext;
+	int			num_retries = 0;
+
+	InitMaterializedSRF(fcinfo, 0);
+
+	/*
+	 * Only superusers or users with pg_read_all_stats privileges can view the
+	 * memory context statistics of another process
+	 */
+	if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("memory context statistics privilege error")));
+
+	/*
+	 * See if the process with given pid is a backend or an auxiliary process.
+	 */
+	proc = BackendPidGetProc(pid);
+	if (proc == NULL)
+		proc = AuxiliaryPidGetProc(pid);
+
+	/*
+	 * BackendPidGetProc() and AuxiliaryPidGetProc() return NULL if the pid
+	 * isn't valid; but by the time we reach kill(), a process for which we
+	 * get a valid proc here might have terminated on its own.  There's no way
+	 * to acquire a lock on an arbitrary process to prevent that. But since
+	 * this mechanism is usually used to debug a backend or an auxiliary
+	 * process running and consuming lots of memory, that it might end on its
+	 * own first and its memory contexts are not logged is not a problem.
+	 */
+	if (proc == NULL)
+	{
+		/*
+		 * This is just a warning so a loop-through-resultset will not abort
+		 * if one backend terminated on its own during the run.
+		 */
+		ereport(WARNING,
+				(errmsg("PID %d is not a PostgreSQL server process", pid)));
+		PG_RETURN_NULL();
+	}
+
+	procNumber = GetNumberFromPGProc(proc);
+	if (procNumber == MyProcNumber)
+	{
+		ereport(WARNING,
+				(errmsg("cannot return statistics for local backend"),
+				 errhint("Use pg_backend_memory_contexts view instead")));
+		PG_RETURN_NULL();
+	}
+
+	/*
+	 * Return statistics of top level 1 and 2 contexts, if get_summary is
+	 * true.
+	 */
+	LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
+	memCtxState[procNumber].get_summary = get_summary;
+
+	/*
+	 * Create a DSA segment with maximum size of 16MB, send handle to the
+	 * publishing process for storing the stats. If number of contexts exceed
+	 * 16MB, a cumulative total is stored for such contexts.
+	 */
+	if (memCtxState[procNumber].memstats_dsa_handle == DSA_HANDLE_INVALID)
+	{
+		oldContext = MemoryContextSwitchTo(TopMemoryContext);
+		area = dsa_create_ext(memCtxState[procNumber].lw_lock.tranche, DSA_DEFAULT_INIT_SEGMENT_SIZE,
+							  16 * DSA_DEFAULT_INIT_SEGMENT_SIZE);
+		MemoryContextSwitchTo(oldContext);
+		handle = dsa_get_handle(area);
+		memCtxState[procNumber].memstats_dsa_handle = handle;
+		/* Pin the mapping so that it doesn't throw a warning */
+		dsa_pin(area);
+		dsa_pin_mapping(area);
+	}
+	else
+	{
+		area = dsa_attach(memCtxState[procNumber].memstats_dsa_handle);
+		dsa_pin_mapping(area);
+	}
+	LWLockRelease(&memCtxState[procNumber].lw_lock);
+	if (SendProcSignal(pid, PROCSIG_GET_MEMORY_CONTEXT, procNumber) < 0)
+	{
+		ereport(WARNING,
+				(errmsg("could not send signal to process %d: %m", pid)));
+		dsa_detach(area);
+		PG_RETURN_NULL();
+	}
+
+	/*
+	 * Wait for a backend to publish stats, indicated by a valid dsa pointer
+	 * set by the backend.
+	 */
+	ConditionVariablePrepareToSleep(&memCtxState[procNumber].memctx_cv);
+	while (1)
+	{
+		proc = BackendPidGetProc(pid);
+		if (proc == NULL)
+			proc = AuxiliaryPidGetProc(pid);
+		if (proc == NULL)
+		{
+			ereport(WARNING,
+					(errmsg("PID %d is not a PostgreSQL server process", pid)));
+			dsa_detach(area);
+			PG_RETURN_NULL();
+		}
+#define MEMSTATS_WAIT_TIMEOUT 5000
+#define MAX_RETRIES 20
+		if (ConditionVariableTimedSleep(&memCtxState[procNumber].memctx_cv, MEMSTATS_WAIT_TIMEOUT,
+										WAIT_EVENT_MEM_CTX_PUBLISH))
+		{
+			ereport(LOG,
+					(errmsg("Wait for %d process to publish stats timed out, trying again", pid)));
+			if (num_retries > MAX_RETRIES)
+			{
+				dsa_detach(area);
+				PG_RETURN_NULL();
+			}
+			num_retries = num_retries + 1;
+		}
+
+		/*
+		 * We expect to come out of sleep when the requested process has
+		 * finished publishing the statistics, verified using the a valid dsa
+		 * pointer.
+		 *
+		 * Make sure that the information belongs to pid we requested
+		 * information for, Otherwise loop back and wait for the server
+		 * process to finish publishing statistics.
+		 */
+		LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
+
+		if (memCtxState[procNumber].proc_id == pid && DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer))
+			break;
+		else
+			LWLockRelease(&memCtxState[procNumber].lw_lock);
+	}
+	if (DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer))
+		memctx_info = (MemoryContextInfo *) dsa_get_address(area, memCtxState[procNumber].memstats_dsa_pointer);
+	/* Backend has finished publishing the stats, read them */
+	for (i = 0; i < memCtxState[procNumber].in_memory_stats; i++)
+	{
+		ArrayType  *path_array;
+		int			path_length;
+		Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
+		bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
+
+		memset(values, 0, sizeof(values));
+		memset(nulls, 0, sizeof(nulls));
+
+		if (strlen(memctx_info[i].name) != 0)
+			values[0] = CStringGetTextDatum(memctx_info[i].name);
+		else
+			nulls[0] = true;
+		if (strlen(memctx_info[i].ident) != 0)
+			values[1] = CStringGetTextDatum(memctx_info[i].ident);
+		else
+			nulls[1] = true;
+
+		values[2] = CStringGetTextDatum(memctx_info[i].type);
+		path_length = memctx_info[i].path_length;
+		path_array = construct_array_builtin(memctx_info[i].path, path_length, INT4OID);
+		values[3] = PointerGetDatum(path_array);
+		values[4] = Int64GetDatum(memctx_info[i].totalspace);
+		values[5] = Int64GetDatum(memctx_info[i].nblocks);
+		values[6] = Int64GetDatum(memctx_info[i].freespace);
+		values[7] = Int64GetDatum(memctx_info[i].freechunks);
+		values[8] = Int64GetDatum(memctx_info[i].totalspace - memctx_info[i].freespace);
+		values[9] = Int32GetDatum(memctx_info[i].num_contexts);
+		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
+	}
+	/* If there are more contexts, display a cumulative total of those */
+	if (memCtxState[procNumber].total_stats > i)
+	{
+		Datum		values[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
+		bool		nulls[PG_GET_BACKEND_MEMORY_CONTEXTS_COLS];
+
+		values[0] = CStringGetTextDatum(memctx_info[i].name);
+		nulls[1] = true;
+		nulls[2] = true;
+		nulls[3] = true;
+		values[4] = Int64GetDatum(memctx_info[i].totalspace);
+		values[5] = Int64GetDatum(memctx_info[i].nblocks);
+		values[6] = Int64GetDatum(memctx_info[i].freespace);
+		values[7] = Int64GetDatum(memctx_info[i].freechunks);
+		values[8] = Int64GetDatum(memctx_info[i].totalspace - memctx_info[i].freespace);
+		values[9] = Int32GetDatum(memctx_info[i].num_contexts);
+		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
+	}
+	LWLockRelease(&memCtxState[procNumber].lw_lock);
+	ConditionVariableCancelSleep();
+	dsa_detach(area);
+	PG_RETURN_NULL();
+}
+
+static Size
+MemCtxShmemSize(void)
+{
+	Size		size;
+	Size		TotalProcs = add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
+
+	size = TotalProcs * sizeof(MemoryContextState);
+	return size;
+}
+
+void
+MemCtxShmemInit(void)
+{
+	bool		found;
+	Size		TotalProcs = add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
+
+	memCtxState = (MemoryContextState *) ShmemInitStruct("MemoryContextState",
+														 MemCtxShmemSize(),
+														 &found);
+	if (!found)
+	{
+		for (int i = 0; i < TotalProcs; i++)
+		{
+			ConditionVariableInit(&memCtxState[i].memctx_cv);
+			LWLockInitialize(&memCtxState[i].lw_lock, LWLockNewTrancheId());
+			LWLockRegisterTranche(memCtxState[i].lw_lock.tranche, "mem_context_stats_reporting");
+			memCtxState[i].memstats_dsa_handle = DSA_HANDLE_INVALID;
+			memCtxState[i].memstats_dsa_pointer = InvalidDsaPointer;
+		}
+	}
+}
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index b844f9fdaef..6bc253da5da 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -42,6 +42,7 @@ volatile sig_atomic_t IdleStatsUpdateTimeoutPending = false;
 volatile uint32 InterruptHoldoffCount = 0;
 volatile uint32 QueryCancelHoldoffCount = 0;
 volatile uint32 CritSectionCount = 0;
+volatile sig_atomic_t PublishMemoryContextPending = false;
 
 int			MyProcPid;
 pg_time_t	MyStartTime;
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index aa6da0d0352..245aba5987c 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -19,16 +19,22 @@
  *-------------------------------------------------------------------------
  */
 
+#include <math.h>
 #include "postgres.h"
 
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "nodes/pg_list.h"
+#include "storage/fd.h"
+#include "storage/lwlock.h"
+#include "storage/dsm.h"
+#include "utils/dsa.h"
+#include "utils/hsearch.h"
 #include "utils/memdebug.h"
 #include "utils/memutils.h"
 #include "utils/memutils_internal.h"
 #include "utils/memutils_memorychunk.h"
 
-
 static void BogusFree(void *pointer);
 static void *BogusRealloc(void *pointer, Size size, int flags);
 static MemoryContext BogusGetChunkContext(void *pointer);
@@ -135,6 +141,12 @@ static const MemoryContextMethods mcxt_methods[] = {
 };
 
 #undef BOGUS_MCTX
+typedef enum PrintDetails
+{
+	PRINT_STATS_TO_STDERR = 0,
+	PRINT_STATS_TO_LOGS,
+	PRINT_STATS_NONE
+}			PrintDetails;
 
 /*
  * CurrentMemoryContext
@@ -162,10 +174,11 @@ static void MemoryContextCallResetCallbacks(MemoryContext context);
 static void MemoryContextStatsInternal(MemoryContext context, int level,
 									   int max_level, int max_children,
 									   MemoryContextCounters *totals,
-									   bool print_to_stderr);
+									   PrintDetails print_location, int *num_contexts);
 static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
 									const char *stats_string,
 									bool print_to_stderr);
+static void PublishMemoryContext(MemoryContextInfo * memctx_infos, int curr_id, MemoryContext context, List *path, char *clipped_ident, MemoryContextCounters stat, int num_contexts);
 
 /*
  * You should not do memory allocations within a critical section, because
@@ -831,11 +844,19 @@ MemoryContextStatsDetail(MemoryContext context,
 						 bool print_to_stderr)
 {
 	MemoryContextCounters grand_totals;
+	int			num_contexts;
+	PrintDetails print_location;
 
 	memset(&grand_totals, 0, sizeof(grand_totals));
 
+	if (print_to_stderr)
+		print_location = PRINT_STATS_TO_STDERR;
+	else
+		print_location = PRINT_STATS_TO_LOGS;
+
+	/* num_contexts report number of contexts aggregated in the output */
 	MemoryContextStatsInternal(context, 0, max_level, max_children,
-							   &grand_totals, print_to_stderr);
+							   &grand_totals, print_location, &num_contexts);
 
 	if (print_to_stderr)
 		fprintf(stderr,
@@ -876,18 +897,43 @@ static void
 MemoryContextStatsInternal(MemoryContext context, int level,
 						   int max_level, int max_children,
 						   MemoryContextCounters *totals,
-						   bool print_to_stderr)
+						   PrintDetails print_location, int *num_contexts)
 {
 	MemoryContext child;
 	int			ichild;
+	bool		print_to_stderr = true;
 
 	Assert(MemoryContextIsValid(context));
 
-	/* Examine the context itself */
-	context->methods->stats(context,
-							MemoryContextStatsPrint,
-							&level,
-							totals, print_to_stderr);
+	if (print_location == PRINT_STATS_TO_STDERR)
+		print_to_stderr = true;
+	else if (print_location == PRINT_STATS_TO_LOGS)
+		print_to_stderr = false;
+
+	if (print_location != PRINT_STATS_NONE)
+	{
+		/* Examine the context itself */
+		context->methods->stats(context,
+								MemoryContextStatsPrint,
+								&level,
+								totals, print_to_stderr);
+	}
+	/* Do not print the statistics */
+
+	/*
+	 * print_to_stderr is a no-op if no statistics are going to be printed i.e
+	 * print_location == PRINT_STATS_NONE
+	 */
+	else
+	{
+		/* Examine the context itself */
+		context->methods->stats(context,
+								NULL,
+								NULL,
+								totals, print_to_stderr);
+	}
+	/* Increment the context count */
+	*num_contexts = *num_contexts + 1;
 
 	/*
 	 * Examine children.
@@ -907,7 +953,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 			MemoryContextStatsInternal(child, level + 1,
 									   max_level, max_children,
 									   totals,
-									   print_to_stderr);
+									   print_location, num_contexts);
 		}
 	}
 
@@ -925,6 +971,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 			ichild++;
 			child = MemoryContextTraverseNext(child, context);
 		}
+		*num_contexts = *num_contexts + ichild;
 
 		if (print_to_stderr)
 		{
@@ -939,7 +986,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
 					local_totals.freechunks,
 					local_totals.totalspace - local_totals.freespace);
 		}
-		else
+		else if (print_location != PRINT_STATS_NONE)
 			ereport(LOG_SERVER_ONLY,
 					(errhidestmt(true),
 					 errhidecontext(true),
@@ -1276,6 +1323,21 @@ HandleLogMemoryContextInterrupt(void)
 	/* latch will be set by procsignal_sigusr1_handler */
 }
 
+/*
+ * HandleGetMemoryContextInterrupt
+ *		Handle receipt of an interrupt indicating publishing of memory
+ *		contexts.
+ *
+ * All the actual work is deferred to ProcessLogMemoryContextInterrupt()
+ */
+void
+HandleGetMemoryContextInterrupt(void)
+{
+	InterruptPending = true;
+	PublishMemoryContextPending = true;
+	/* latch will be set by procsignal_sigusr1_handler */
+}
+
 /*
  * ProcessLogMemoryContextInterrupt
  * 		Perform logging of memory contexts of this backend process.
@@ -1313,6 +1375,346 @@ ProcessLogMemoryContextInterrupt(void)
 	MemoryContextStatsDetail(TopMemoryContext, 100, 100, false);
 }
 
+/*
+ * Run by each backend to publish their memory context
+ * statistics. It performs a breadth first search
+ * on the memory context tree, so that the parents
+ * get a chance to report stats before their children.
+ *
+ * Statistics are shared via dynamic shared memory which
+ * can hold statistics of approx 6700 contexts. Remaining
+ * contexts statistics is captured as a cumulative total.
+ */
+void
+ProcessGetMemoryContextInterrupt(void)
+{
+	/* Store the memory context details in shared memory */
+
+	List	   *contexts;
+
+	HASHCTL		ctl;
+	HTAB	   *context_id_lookup;
+	int			context_id = 0;
+	bool		found;
+	MemoryContext stat_cxt;
+	MemoryContextInfo *meminfo;
+	bool		get_summary = false;
+	dsa_area   *area;
+	int			num_stats;
+	int			idx = MyProcNumber;
+	int			stats_count = 0;
+	MemoryContextCounters stat;
+
+	PublishMemoryContextPending = false;
+
+	/*
+	 * The hash table is used for constructing "path" column of
+	 * pg_get_process_memory_context is view, similar to its local backend
+	 * couterpart.
+	 */
+
+	/*
+	 * Make a new context that will contain the hash table, to ease the
+	 * cleanup
+	 */
+
+	stat_cxt = AllocSetContextCreate(CurrentMemoryContext,
+									 "Memory context statistics",
+									 ALLOCSET_DEFAULT_SIZES);
+
+	ctl.keysize = sizeof(MemoryContext);
+	ctl.entrysize = sizeof(MemoryContextId);
+	ctl.hcxt = stat_cxt;
+
+	context_id_lookup = hash_create("pg_get_remote_backend_memory_contexts",
+									256,
+									&ctl,
+									HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+	contexts = list_make1(TopMemoryContext);
+
+	/* Compute the number of stats that can fit in the DSM seg */
+	num_stats = floor(16 * DSA_DEFAULT_INIT_SEGMENT_SIZE / sizeof(MemoryContextInfo));
+
+	/*
+	 * Traverse the memory context tree to find total number of contexts. If
+	 * summary is requested find the total number of contexts at level 1 and
+	 * 2.
+	 */
+	foreach_ptr(MemoryContextData, cur, contexts)
+	{
+		MemoryContextId *entry;
+
+		entry = (MemoryContextId *) hash_search(context_id_lookup, &cur,
+												HASH_ENTER, &found);
+		stats_count = stats_count + 1;
+		/* context id starts with 1 */
+		entry->context_id = stats_count;
+
+		/* Append the children of the current context to the main list */
+		for (MemoryContext c = cur->firstchild; c != NULL; c = c->nextchild)
+		{
+			if (get_summary)
+			{
+				entry = (MemoryContextId *) hash_search(context_id_lookup, &c,
+														HASH_ENTER, &found);
+				stats_count = stats_count + 1;
+				entry->context_id = stats_count;
+			}
+			contexts = lappend(contexts, c);
+		}
+		/* In summary only the first level contexts are displayed */
+		if (get_summary)
+			break;
+	}
+
+	/*
+	 * Allocate memory in this process's dsa for storing statistics of the the
+	 * memory contexts upto num_stats, for contexts that don't fit in the DSA
+	 * segment, a cumulative total is written as the last record in the DSA
+	 * segment.
+	 */
+	stats_count = stats_count > num_stats ? num_stats : stats_count;
+
+	/* Attach to DSA segment */
+	LWLockAcquire(&memCtxState[idx].lw_lock, LW_EXCLUSIVE);
+	area = dsa_attach(memCtxState[idx].memstats_dsa_handle);
+	memCtxState[idx].proc_id = MyProcPid;
+	get_summary = memCtxState[idx].get_summary;
+
+	/* Free the memory allocated previously by the same process */
+	if (DsaPointerIsValid(memCtxState[idx].memstats_dsa_pointer))
+	{
+		dsa_free(area, memCtxState[idx].memstats_dsa_pointer);
+		memCtxState[idx].memstats_dsa_pointer = InvalidDsaPointer;
+	}
+	memCtxState[idx].memstats_dsa_pointer = dsa_allocate0(area, stats_count * sizeof(MemoryContextInfo));
+	meminfo = (MemoryContextInfo *) dsa_get_address(area, memCtxState[idx].memstats_dsa_pointer);
+
+	if (get_summary)
+	{
+		int			ctx_id = 0;
+		List	   *path = NIL;
+
+		/* Copy TopMemoryContext statistics to DSA */
+		path = lcons_int(1, path);
+		memset(&stat, 0, sizeof(stat));
+		(*TopMemoryContext->methods->stats) (TopMemoryContext, NULL, NULL, &stat, true);
+		PublishMemoryContext(meminfo, ctx_id, TopMemoryContext, path, NULL, stat, 1);
+		ctx_id = ctx_id + 1;
+
+		/*
+		 * Copy statistics for each of TopMemoryContexts children(XXX. Make it
+		 * capped at 100). This includes statistics of all of their children
+		 * upto level 100
+		 */
+		for (MemoryContext c = TopMemoryContext->firstchild; c != NULL; c = c->nextchild)
+		{
+			MemoryContextCounters grand_totals;
+			int			num_contexts = 0;
+			char		clipped_ident[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];
+
+			path = NIL;
+			memset(&grand_totals, 0, sizeof(grand_totals));
+
+			MemoryContextStatsInternal(c, 0, 100, 100, &grand_totals, PRINT_STATS_NONE, &num_contexts);
+
+			/*
+			 * Figure out the transient context_id of this context and each of
+			 * its ancestors.
+			 */
+			for (MemoryContext cur_context = c; cur_context != NULL; cur_context = cur_context->parent)
+			{
+				MemoryContextId *cur_entry;
+
+				cur_entry = hash_search(context_id_lookup, &cur_context, HASH_FIND, &found);
+
+				if (!found)
+				{
+					elog(LOG, "hash table corrupted, can't construct path value");
+					break;
+				}
+				path = lcons_int(cur_entry->context_id, path);
+			}
+			/* Trim and copy the identifier if it is not set to NULL */
+			if (c->ident != NULL)
+			{
+				int			idlen = strlen(c->ident);
+
+				/*
+				 * Some identifiers such as SQL query string can be very long,
+				 * truncate oversize identifiers.
+				 */
+				if (idlen >= MEMORY_CONTEXT_IDENT_DISPLAY_SIZE)
+					idlen = pg_mbcliplen(c->ident, idlen, MEMORY_CONTEXT_IDENT_DISPLAY_SIZE - 1);
+
+				memcpy(clipped_ident, c->ident, idlen);
+				clipped_ident[idlen] = '\0';
+			}
+			PublishMemoryContext(meminfo, ctx_id, c, path, (c->ident != NULL ? clipped_ident : NULL), grand_totals, num_contexts);
+			ctx_id = ctx_id + 1;
+		}
+		/* For summary mode, total_stats and in_memory_stats remain the same */
+		memCtxState[idx].in_memory_stats = ctx_id;
+		memCtxState[idx].total_stats = ctx_id;
+		goto cleanup;
+	}
+
+	foreach_ptr(MemoryContextData, cur, contexts)
+	{
+		List	   *path = NIL;
+		char		clipped_ident[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];
+
+		/*
+		 * Figure out the transient context_id of this context and each of its
+		 * ancestors.
+		 */
+		for (MemoryContext cur_context = cur; cur_context != NULL; cur_context = cur_context->parent)
+		{
+			MemoryContextId *cur_entry;
+
+			cur_entry = hash_search(context_id_lookup, &cur_context, HASH_FIND, &found);
+
+			if (!found)
+			{
+				elog(LOG, "hash table corrupted, can't construct path value");
+				break;
+			}
+			path = lcons_int(cur_entry->context_id, path);
+		}
+		/* Trim and copy the identifier if it is not set to NULL */
+		if (cur->ident != NULL)
+		{
+			int			idlen = strlen(cur->ident);
+
+			/*
+			 * Some identifiers such as SQL query string can be very long,
+			 * truncate oversize identifiers.
+			 */
+			if (idlen >= MEMORY_CONTEXT_IDENT_DISPLAY_SIZE)
+				idlen = pg_mbcliplen(cur->ident, idlen, MEMORY_CONTEXT_IDENT_DISPLAY_SIZE - 1);
+
+			memcpy(clipped_ident, cur->ident, idlen);
+			clipped_ident[idlen] = '\0';
+		}
+		if (context_id <= (num_stats - 2))
+		{
+			/* Examine the context stats */
+			memset(&stat, 0, sizeof(stat));
+			(*cur->methods->stats) (cur, NULL, NULL, &stat, true);
+			/* Copy statistics to DSM memory */
+			PublishMemoryContext(meminfo, context_id, cur, path, (cur->ident != NULL ? clipped_ident : NULL), stat, 1);
+		}
+		else
+		{
+			/* Examine the context stats */
+			memset(&stat, 0, sizeof(stat));
+			(*cur->methods->stats) (cur, NULL, NULL, &stat, true);
+
+			meminfo[num_stats - 1].totalspace += stat.totalspace;
+			meminfo[num_stats - 1].nblocks += stat.nblocks;
+			meminfo[num_stats - 1].freespace += stat.freespace;
+			meminfo[num_stats - 1].freechunks += stat.freechunks;
+		}
+
+		/*
+		 * DSA max limit is reached, write total of the remaining statistics.
+		 */
+		if (context_id == (num_stats - 2) && context_id < (stats_count - 1))
+		{
+			memCtxState[idx].in_memory_stats = context_id + 1;
+			strncpy(meminfo[num_stats - 1].name, "Remaining Totals", 16);
+		}
+		context_id++;
+	}
+	if (context_id < (num_stats - 2))
+	{
+		memCtxState[idx].in_memory_stats = context_id;
+	}
+	/* Report number of aggregated memory contexts */
+	else
+	{
+		meminfo[num_stats - 1].num_contexts = context_id - memCtxState[idx].in_memory_stats;
+	}
+	memCtxState[idx].total_stats = context_id;
+cleanup:
+
+	/*
+	 * Signal all the waiting client backends after setting the exit condition
+	 * flag
+	 */
+	LWLockRelease(&memCtxState[idx].lw_lock);
+	ConditionVariableBroadcast(&memCtxState[idx].memctx_cv);
+	/* Delete the hash table memory context */
+	MemoryContextDelete(stat_cxt);
+
+	dsa_detach(area);
+}
+
+static void
+PublishMemoryContext(MemoryContextInfo * memctx_info, int curr_id, MemoryContext context, List *path, char *clipped_ident, MemoryContextCounters stat, int num_contexts)
+{
+	char	   *type;
+
+	if (context->name != NULL)
+	{
+		Assert(strlen(context->name) < MEMORY_CONTEXT_IDENT_DISPLAY_SIZE);
+		strncpy(memctx_info[curr_id].name, context->name, strlen(context->name));
+	}
+	else
+		memctx_info[curr_id].name[0] = '\0';
+
+	if (clipped_ident != NULL)
+	{
+		/*
+		 * To be consistent with logging output, we label dynahash contexts
+		 * with just the hash table name as with MemoryContextStatsPrint().
+		 */
+		if (!strncmp(context->name, "dynahash", 8))
+		{
+			strncpy(memctx_info[curr_id].name, clipped_ident, strlen(clipped_ident));
+			memctx_info[curr_id].ident[0] = '\0';
+		}
+		else
+			strncpy(memctx_info[curr_id].ident, clipped_ident, strlen(clipped_ident));
+	}
+	else
+		memctx_info[curr_id].ident[0] = '\0';
+
+	memctx_info[curr_id].path_length = list_length(path);
+	foreach_int(i, path)
+		memctx_info[curr_id].path[foreach_current_index(i)] = Int32GetDatum(i);
+
+	switch (context->type)
+	{
+		case T_AllocSetContext:
+			type = "AllocSet";
+			strncpy(memctx_info[curr_id].type, type, strlen(type));
+			break;
+		case T_GenerationContext:
+			type = "Generation";
+			strncpy(memctx_info[curr_id].type, type, strlen(type));
+			break;
+		case T_SlabContext:
+			type = "Slab";
+			strncpy(memctx_info[curr_id].type, type, strlen(type));
+			break;
+		case T_BumpContext:
+			type = "Bump";
+			strncpy(memctx_info[curr_id].type, type, strlen(type));
+			break;
+		default:
+			type = "???";
+			strncpy(memctx_info[curr_id].type, type, strlen(type));
+			break;
+	}
+	memctx_info[curr_id].totalspace = stat.totalspace;
+	memctx_info[curr_id].nblocks = stat.nblocks;
+	memctx_info[curr_id].freespace = stat.freespace;
+	memctx_info[curr_id].freechunks = stat.freechunks;
+	memctx_info[curr_id].num_contexts = num_contexts;
+}
+
 void *
 palloc(Size size)
 {
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index b37e8a6f882..4d6ae0728ac 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8449,6 +8449,16 @@
   prorettype => 'bool', proargtypes => 'int4',
   prosrc => 'pg_log_backend_memory_contexts' },
 
+# publishing memory contexts of the specified postgres process
+{ oid => '2173', descr => 'publish memory contexts of the specified backend',
+  proname => 'pg_get_process_memory_contexts', provolatile => 'v',
+  prorows => '100', proretset => 't', proparallel => 'r',
+  prorettype => 'record', proargtypes => 'int4 bool',
+  proallargtypes => '{int4,bool,text,text,text,_int4,int8,int8,int8,int8,int8,int4}',
+  proargmodes => '{i,i,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{oid, summary, name, ident, type, path, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes, num_agg_contexts}',
+  prosrc => 'pg_get_process_memory_contexts' },
+
 # non-persistent series generator
 { oid => '1066', descr => 'non-persistent series generator',
   proname => 'generate_series', prorows => '1000',
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index d016a9c9248..fc75ea143c3 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -96,6 +96,7 @@ extern PGDLLIMPORT volatile sig_atomic_t IdleSessionTimeoutPending;
 extern PGDLLIMPORT volatile sig_atomic_t ProcSignalBarrierPending;
 extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending;
 extern PGDLLIMPORT volatile sig_atomic_t IdleStatsUpdateTimeoutPending;
+extern PGDLLIMPORT volatile sig_atomic_t PublishMemoryContextPending;
 
 extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending;
 extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost;
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 022fd8ed933..477ab993386 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -35,6 +35,7 @@ typedef enum
 	PROCSIG_WALSND_INIT_STOPPING,	/* ask walsenders to prepare for shutdown  */
 	PROCSIG_BARRIER,			/* global barrier interrupt  */
 	PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
+	PROCSIG_GET_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
 	PROCSIG_PARALLEL_APPLY_MESSAGE, /* Message from parallel apply workers */
 
 	/* Recovery conflict reasons */
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 8abc26abce2..9fac394aad3 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -18,6 +18,9 @@
 #define MEMUTILS_H
 
 #include "nodes/memnodes.h"
+#include "storage/condition_variable.h"
+#include "storage/lmgr.h"
+#include "utils/dsa.h"

 /*
@@ -48,7 +51,11 @@
 
 #define AllocHugeSizeIsValid(size)	((Size) (size) <= MaxAllocHugeSize)
 
+#define MEMORY_CONTEXT_IDENT_DISPLAY_SIZE	1024
 
+#define MEM_CONTEXT_SHMEM_STATS_SIZE	30
+#define MEM_CONTEXT_MAX_LEVEL	64
+#define MAX_TYPE_STRING_LENGTH	64
 /*
  * Standard top-level memory contexts.
  *
@@ -115,6 +122,50 @@ extern MemoryContext AllocSetContextCreateInternal(MemoryContext parent,
 												   Size initBlockSize,
 												   Size maxBlockSize);
 
+/* Dynamic shared memory state for Memory Context Statistics reporting */
+typedef struct MemoryContextInfo
+{
+	char		name[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];
+	char		ident[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];
+	Datum		path[MEM_CONTEXT_MAX_LEVEL];
+	char		type[MAX_TYPE_STRING_LENGTH];
+	int			path_length;
+	int64		totalspace;
+	int64		nblocks;
+	int64		freespace;
+	int64		freechunks;
+	int			num_contexts;
+}			MemoryContextInfo;
+
+typedef struct MemoryContextState
+{
+	ConditionVariable memctx_cv;
+	LWLock		lw_lock;
+	int			proc_id;
+	int			in_memory_stats;
+	int			total_stats;
+	bool		get_summary;
+	dsa_handle	memstats_dsa_handle;
+	dsa_pointer memstats_dsa_pointer;
+
+}			MemoryContextState;
+
+/*
+ * MemoryContextId
+ *		Used for storage of transient identifiers for
+ *		pg_get_backend_memory_contexts.
+ */
+typedef struct MemoryContextId
+{
+	MemoryContext context;
+	int			context_id;
+}			MemoryContextId;
+
+extern PGDLLIMPORT MemoryContextState * memCtxState;
+extern void ProcessGetMemoryContextInterrupt(void);
+extern void HandleGetMemoryContextInterrupt(void);
+extern void MemCtxShmemInit(void);
+
 /*
  * This wrapper macro exists to check for non-constant strings used as context
  * names; that's no longer supported.  (Use MemoryContextSetIdentifier if you
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 91089ac215f..5e3382132c3 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -223,3 +223,15 @@ select count(distinct utc_offset) >= 24 as ok from pg_timezone_abbrevs;
  t
 (1 row)
 
+DO $$
+DECLARE
+    checkpointer_pid int;
+    r RECORD;
+BEGIN
+        SELECT pid from pg_stat_activity where backend_type='checkpointer' INTO checkpointer_pid;
+
+        select type, name, ident
+        from pg_get_process_memory_contexts(checkpointer_pid, false) where path = '{0}' into r;
+	RAISE NOTICE '%', r;
+END $$;
+NOTICE:  (AllocSet,TopMemoryContext,)
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sql
index b2a79237543..f3127fea400 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -98,3 +98,15 @@ set timezone_abbreviations = 'Australia';
 select count(distinct utc_offset) >= 24 as ok from pg_timezone_abbrevs;
 set timezone_abbreviations = 'India';
 select count(distinct utc_offset) >= 24 as ok from pg_timezone_abbrevs;
+
+DO $$
+DECLARE
+    checkpointer_pid int;
+    r RECORD;
+BEGIN
+        SELECT pid from pg_stat_activity where backend_type='checkpointer' INTO checkpointer_pid;
+
+        select type, name, ident
+        from pg_get_process_memory_contexts(checkpointer_pid, false) where path = '{0}' into r;
+	RAISE NOTICE '%', r;
+END $$;
-- 
2.47.1

From 0ef536a26403973b081fd14de79e047cd0cd1a13 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email-masked]>
Date: Mon, 6 Jan 2025 19:22:14 +0100
Subject: [PATCH vtomas 2/2] review

---
 src/backend/utils/adt/mcxtfuncs.c | 109 +++++++++++++++++++++++++-----
 src/backend/utils/mmgr/mcxt.c     |  93 +++++++++++++++++--------
 src/include/utils/memutils.h      |   5 +-
 3 files changed, 159 insertions(+), 48 deletions(-)

diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index c067cdf8709..f3ffd6937a5 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -307,16 +307,26 @@ pg_log_backend_memory_contexts(PG_FUNCTION_ARGS)
  * pg_get_process_memory_contexts
  *		Signal a backend or an auxiliary process to send its memory contexts.
  *
+ * By default, only superusers are allowed to signal to return the memory
+ * contexts because allowing any users to issue this request at an unbounded
+ * rate would cause lots of log messages and which can lead to denial of
+ * service. Additional roles can be permitted with GRANT.
+ *
  * On receipt of this signal, a backend or an auxiliary process sets the flag
  * in the signal handler, which causes the next CHECK_FOR_INTERRUPTS()
- * or process-specific interrupt handler to copy the memory context statistics
- * in a dynamic shared memory space. The statistics for contexts that do not fit in
- * shared memory area are stored as a cumulative total of those contexts,
- * at the end in the dynamic shared memory.
- * Wait for the backend to send signal on the condition variable after
- * writing statistics to a shared memory.
- * Once condition variable comes out of sleep, check if the required
- * backends statistics are available to read and display.
+ * or process-specific interrupt handler to copy the memory context details
+ * to a dynamic shared memory space.
+ *
+ * The shared memory buffer has a limited size - it the process has too many
+ * memory contexts, the memory contexts into that do not fit are summarized
+ * and represented as cumulative total at the end of the buffer.
+ *
+ * Once condition variable comes out of sleep, check if the memory context
+ * information is available for read and display.
+ *
+ * XXX Explain how the backends communicate through condition variables.
+ *
+ * XXX Explain what happens with timeouts, etc.
  */
 Datum
 pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
@@ -333,6 +343,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 	MemoryContext oldContext;
 	int			num_retries = 0;
 
+	/* XXX Shouldn't this be after the privilege check etc.? */
 	InitMaterializedSRF(fcinfo, 0);
 
 	/*
@@ -383,24 +394,50 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 	/*
 	 * Return statistics of top level 1 and 2 contexts, if get_summary is
 	 * true.
+	 *
+	 * XXX Is this comment still accurate? Or are we returning information
+	 * about more contexts? Or more precisely, isn't that irrelevant here?
+	 * We just process whatever the process puts into the DSA, right?
+	 *
+	 * XXX I'd move this comment until after we wake up and are ready to
+	 * process the information, close to the comment:
+	 *
+	 *		Backend has finished publishing the stats, read them
 	 */
-	LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
-	memCtxState[procNumber].get_summary = get_summary;
 
 	/*
 	 * Create a DSA segment with maximum size of 16MB, send handle to the
 	 * publishing process for storing the stats. If number of contexts exceed
 	 * 16MB, a cumulative total is stored for such contexts.
+	 *
+	 * XXX 16MB seems like an awfully large amount of memory, particularly
+	 * for small machines. Maybe it should be configurable as a parameter
+	 * of the SQL function? In any case, it should not be hardcoded as a
+	 * magic constant. Maybe add a #define constant?
 	 */
+	LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
+
+	memCtxState[procNumber].get_summary = get_summary;
+
 	if (memCtxState[procNumber].memstats_dsa_handle == DSA_HANDLE_INVALID)
 	{
 		oldContext = MemoryContextSwitchTo(TopMemoryContext);
-		area = dsa_create_ext(memCtxState[procNumber].lw_lock.tranche, DSA_DEFAULT_INIT_SEGMENT_SIZE,
+
+		area = dsa_create_ext(memCtxState[procNumber].lw_lock.tranche,
+							  DSA_DEFAULT_INIT_SEGMENT_SIZE,
 							  16 * DSA_DEFAULT_INIT_SEGMENT_SIZE);
+
 		MemoryContextSwitchTo(oldContext);
+
 		handle = dsa_get_handle(area);
+
 		memCtxState[procNumber].memstats_dsa_handle = handle;
-		/* Pin the mapping so that it doesn't throw a warning */
+
+		/* Pin the mapping so that it doesn't throw a warning
+		 *
+		 * XXX We don't pin stuff "so that it doesn't throw a warning". Surely
+		 * the warning has a reason, so maybe mention that?
+		 */
 		dsa_pin(area);
 		dsa_pin_mapping(area);
 	}
@@ -409,11 +446,20 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 		area = dsa_attach(memCtxState[procNumber].memstats_dsa_handle);
 		dsa_pin_mapping(area);
 	}
+
 	LWLockRelease(&memCtxState[procNumber].lw_lock);
+
+	/*
+	 * Send a signal to the auxiliary process, informing it we want it to
+	 * produce information about memory contexts.
+	 */
 	if (SendProcSignal(pid, PROCSIG_GET_MEMORY_CONTEXT, procNumber) < 0)
 	{
 		ereport(WARNING,
 				(errmsg("could not send signal to process %d: %m", pid)));
+
+		/* XXX We do exactly this in a number of places. Maybe it'd be better
+		 * to define an "error" label at the end, and goto to it? */
 		dsa_detach(area);
 		PG_RETURN_NULL();
 	}
@@ -461,13 +507,22 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 		 */
 		LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
 
-		if (memCtxState[procNumber].proc_id == pid && DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer))
+		/*
+		 * XXX Explain how could it happen that the PID does not match.
+		 */
+		if ((memCtxState[procNumber].proc_id == pid) &&
+			DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer))
 			break;
 		else
 			LWLockRelease(&memCtxState[procNumber].lw_lock);
 	}
+
 	if (DsaPointerIsValid(memCtxState[procNumber].memstats_dsa_pointer))
 		memctx_info = (MemoryContextInfo *) dsa_get_address(area, memCtxState[procNumber].memstats_dsa_pointer);
+
+	/* XXX What if the memstats_dsa_pointer is not valid? Is it even possible? 
+	 * If it is, we have garbage in memctx_info. Maybe should be an Assert()? */
+
 	/* Backend has finished publishing the stats, read them */
 	for (i = 0; i < memCtxState[procNumber].in_memory_stats; i++)
 	{
@@ -489,17 +544,21 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 			nulls[1] = true;
 
 		values[2] = CStringGetTextDatum(memctx_info[i].type);
+
 		path_length = memctx_info[i].path_length;
 		path_array = construct_array_builtin(memctx_info[i].path, path_length, INT4OID);
 		values[3] = PointerGetDatum(path_array);
+
 		values[4] = Int64GetDatum(memctx_info[i].totalspace);
 		values[5] = Int64GetDatum(memctx_info[i].nblocks);
 		values[6] = Int64GetDatum(memctx_info[i].freespace);
 		values[7] = Int64GetDatum(memctx_info[i].freechunks);
 		values[8] = Int64GetDatum(memctx_info[i].totalspace - memctx_info[i].freespace);
 		values[9] = Int32GetDatum(memctx_info[i].num_contexts);
+
 		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
 	}
+
 	/* If there are more contexts, display a cumulative total of those */
 	if (memCtxState[procNumber].total_stats > i)
 	{
@@ -516,29 +575,41 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 		values[7] = Int64GetDatum(memctx_info[i].freechunks);
 		values[8] = Int64GetDatum(memctx_info[i].totalspace - memctx_info[i].freespace);
 		values[9] = Int32GetDatum(memctx_info[i].num_contexts);
+
 		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
 	}
+
 	LWLockRelease(&memCtxState[procNumber].lw_lock);
+
 	ConditionVariableCancelSleep();
+
 	dsa_detach(area);
 	PG_RETURN_NULL();
 }
 
+/*
+ * Shared memory sizing for reporting memory context information.
+ */
 static Size
 MemCtxShmemSize(void)
 {
-	Size		size;
-	Size		TotalProcs = add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
+	Size		TotalProcs =
+		add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
 
-	size = TotalProcs * sizeof(MemoryContextState);
-	return size;
+	return mul_size(TotalProcs, sizeof(MemoryContextState));
 }
 
+/*
+ * Init shared memory for reporting memory context information.
+ *
+ * XXX Should this check IsUnderPostmaster, similarly to e.g. CommitTsShmemInit?
+ */
 void
 MemCtxShmemInit(void)
 {
 	bool		found;
-	Size		TotalProcs = add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
+	Size		TotalProcs =
+		add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
 
 	memCtxState = (MemoryContextState *) ShmemInitStruct("MemoryContextState",
 														 MemCtxShmemSize(),
@@ -548,8 +619,10 @@ MemCtxShmemInit(void)
 		for (int i = 0; i < TotalProcs; i++)
 		{
 			ConditionVariableInit(&memCtxState[i].memctx_cv);
+
 			LWLockInitialize(&memCtxState[i].lw_lock, LWLockNewTrancheId());
 			LWLockRegisterTranche(memCtxState[i].lw_lock.tranche, "mem_context_stats_reporting");
+
 			memCtxState[i].memstats_dsa_handle = DSA_HANDLE_INVALID;
 			memCtxState[i].memstats_dsa_pointer = InvalidDsaPointer;
 		}
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 245aba5987c..8992a01ee32 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -178,7 +178,11 @@ static void MemoryContextStatsInternal(MemoryContext context, int level,
 static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
 									const char *stats_string,
 									bool print_to_stderr);
-static void PublishMemoryContext(MemoryContextInfo * memctx_infos, int curr_id, MemoryContext context, List *path, char *clipped_ident, MemoryContextCounters stat, int num_contexts);
+static void PublishMemoryContext(MemoryContextInfo * memctx_infos,
+								 int curr_id, MemoryContext context,
+								 List *path, char *clipped_ident,
+								 MemoryContextCounters stat,
+								 int num_contexts);
 
 /*
  * You should not do memory allocations within a critical section, because
@@ -1376,20 +1380,22 @@ ProcessLogMemoryContextInterrupt(void)
 }
 
 /*
- * Run by each backend to publish their memory context
- * statistics. It performs a breadth first search
- * on the memory context tree, so that the parents
- * get a chance to report stats before their children.
+ * ProcessGetMemoryContextInterrupt
+ *		Generate information about memory contexts used by the process.
  *
- * Statistics are shared via dynamic shared memory which
- * can hold statistics of approx 6700 contexts. Remaining
- * contexts statistics is captured as a cumulative total.
+ * Performs a breadth first search on the memory context tree, so that the
+ * parents get a chance to report stats before their children.
+ *
+ * Statistics are shared via dynamic shared memory which can hold statistics
+ * of approx 6700 contexts. Remaining contexts statistics is captured as a
+ * cumulative total.
+ *
+ * XXX Seems a bit fragile to mention the number of contexts here - if the
+ * DSA size changes (in mcxtfuncs.c), this will get stale.
  */
 void
 ProcessGetMemoryContextInterrupt(void)
 {
-	/* Store the memory context details in shared memory */
-
 	List	   *contexts;
 
 	HASHCTL		ctl;
@@ -1407,21 +1413,18 @@ ProcessGetMemoryContextInterrupt(void)
 
 	PublishMemoryContextPending = false;
 
-	/*
-	 * The hash table is used for constructing "path" column of
-	 * pg_get_process_memory_context is view, similar to its local backend
-	 * couterpart.
-	 */
-
 	/*
 	 * Make a new context that will contain the hash table, to ease the
-	 * cleanup
+	 * cleanup.
 	 */
-
 	stat_cxt = AllocSetContextCreate(CurrentMemoryContext,
 									 "Memory context statistics",
 									 ALLOCSET_DEFAULT_SIZES);
 
+	/*
+	 * The hash table used for constructing "path" column of the view, similar
+	 * to its local backend counterpart.
+	 */
 	ctl.keysize = sizeof(MemoryContext);
 	ctl.entrysize = sizeof(MemoryContextId);
 	ctl.hcxt = stat_cxt;
@@ -1431,39 +1434,65 @@ ProcessGetMemoryContextInterrupt(void)
 									&ctl,
 									HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
 
+	/* List of contexts to process in the next round - start at the top. */
 	contexts = list_make1(TopMemoryContext);
 
 	/* Compute the number of stats that can fit in the DSM seg */
+	/*
+	 * XXX Don't hardcode the size in two places. Define a constant or
+	 * something like that, so that we can change one place and it stays
+	 * in sync. Or even better, define it just in mcxtfuncs.c, and store
+	 * the size in the shmem.
+	 *
+	 * XXX The name is misleading - this is not the number of stats we're
+	 * about to produce, it's the maximum number of entries we can fit into
+	 * the shmem. I'd name this max_stats.
+	 *
+	 * XXX Also, what if we fill exactly this number of contexts? Won't we
+	 * lose the last entry because it will be overwitten by the summary?
+	 */
 	num_stats = floor(16 * DSA_DEFAULT_INIT_SEGMENT_SIZE / sizeof(MemoryContextInfo));
 
 	/*
 	 * Traverse the memory context tree to find total number of contexts. If
 	 * summary is requested find the total number of contexts at level 1 and
 	 * 2.
+	 *
+	 * XXX I'm confused about how this interacts with the get_summary flag.
+	 * In fact, this always uses get_summary=false, because we only read the
+	 * flag from shmem later. Seems like a bug.
 	 */
 	foreach_ptr(MemoryContextData, cur, contexts)
 	{
-		MemoryContextId *entry;
+		MemoryContextId    *entry;
 
 		entry = (MemoryContextId *) hash_search(context_id_lookup, &cur,
 												HASH_ENTER, &found);
-		stats_count = stats_count + 1;
+		Assert(!found);
+
 		/* context id starts with 1 */
-		entry->context_id = stats_count;
+		entry->context_id = (++stats_count);
 
-		/* Append the children of the current context to the main list */
+		/* Append the children of the current context to the main list. */
 		for (MemoryContext c = cur->firstchild; c != NULL; c = c->nextchild)
 		{
+			/* XXX I don't understand why we need to check get_summary here? */
 			if (get_summary)
 			{
 				entry = (MemoryContextId *) hash_search(context_id_lookup, &c,
 														HASH_ENTER, &found);
-				stats_count = stats_count + 1;
-				entry->context_id = stats_count;
+				Assert(!found);
+
+				entry->context_id = (++stats_count);
 			}
+
 			contexts = lappend(contexts, c);
 		}
-		/* In summary only the first level contexts are displayed */
+
+		/* In summary only the first level contexts are displayed
+		 *
+		 * XXX Probably should say "first two levels"?
+		 */
 		if (get_summary)
 			break;
 	}
@@ -1474,23 +1503,30 @@ ProcessGetMemoryContextInterrupt(void)
 	 * segment, a cumulative total is written as the last record in the DSA
 	 * segment.
 	 */
-	stats_count = stats_count > num_stats ? num_stats : stats_count;
+	stats_count = (stats_count > num_stats) ? num_stats : stats_count;
 
 	/* Attach to DSA segment */
 	LWLockAcquire(&memCtxState[idx].lw_lock, LW_EXCLUSIVE);
 	area = dsa_attach(memCtxState[idx].memstats_dsa_handle);
+
 	memCtxState[idx].proc_id = MyProcPid;
+
+	/* XXX this is where we get the get_summary flag */
 	get_summary = memCtxState[idx].get_summary;
 
-	/* Free the memory allocated previously by the same process */
+	/* Free the memory allocated previously by the same process. */
 	if (DsaPointerIsValid(memCtxState[idx].memstats_dsa_pointer))
 	{
 		dsa_free(area, memCtxState[idx].memstats_dsa_pointer);
 		memCtxState[idx].memstats_dsa_pointer = InvalidDsaPointer;
 	}
+
 	memCtxState[idx].memstats_dsa_pointer = dsa_allocate0(area, stats_count * sizeof(MemoryContextInfo));
 	meminfo = (MemoryContextInfo *) dsa_get_address(area, memCtxState[idx].memstats_dsa_pointer);
 
+	/* XXX I find this really hard to understand, with the nested loops etc.
+	 * I suggest breaking this up into smaller functions, and calling them
+	 * (easier to understand) than huge lump of code. */
 	if (get_summary)
 	{
 		int			ctx_id = 0;
@@ -1506,7 +1542,7 @@ ProcessGetMemoryContextInterrupt(void)
 		/*
 		 * Copy statistics for each of TopMemoryContexts children(XXX. Make it
 		 * capped at 100). This includes statistics of all of their children
-		 * upto level 100
+		 * upto level 100.
 		 */
 		for (MemoryContext c = TopMemoryContext->firstchild; c != NULL; c = c->nextchild)
 		{
@@ -1651,6 +1687,7 @@ cleanup:
 	dsa_detach(area);
 }
 
+/* XXX this really needs some better formatting and comments */
 static void
 PublishMemoryContext(MemoryContextInfo * memctx_info, int curr_id, MemoryContext context, List *path, char *clipped_ident, MemoryContextCounters stat, int num_contexts)
 {
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 9fac394aad3..49c7bf5c376 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -123,8 +123,9 @@ extern MemoryContext AllocSetContextCreateInternal(MemoryContext parent,
 												   Size maxBlockSize);
 
 /* Dynamic shared memory state for Memory Context Statistics reporting */
-typedef struct MemoryContextInfo
+typedef struct MemoryContextInfo	/* XXX I'd name this MemoryContextEntry */
 {
+	/* XXX isn't 2 x 1kB for every context a bit too much? Maybe better to make it variable-length? */
 	char		name[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];
 	char		ident[MEMORY_CONTEXT_IDENT_DISPLAY_SIZE];
 	Datum		path[MEM_CONTEXT_MAX_LEVEL];
@@ -135,7 +136,7 @@ typedef struct MemoryContextInfo
 	int64		freespace;
 	int64		freechunks;
 	int			num_contexts;
-}			MemoryContextInfo;
+}			MemoryContextInfo;	/* XXX needs to be added to typedefs, so that pgindent works */
 
 typedef struct MemoryContextState
 {
-- 
2.47.1

---

From: Rahila Syed <[email-masked]>
Date: Wed, 8 Jan 2025 17:33:01 +0530

Hi Fujii-san,

Thank you for testing the feature.

> In this scenario, a DSM segment descriptor is created and associated with
the
CurrentResourceOwner, which is set to the aborting transaction's resource
owner.
This occurs when ProcessGetMemoryContextInterrupts is called by the backend
while a transaction is still open and about to be rolled back.

I believe this issue needs to be addressed in the DSA and DSM code by
adding
a check to ensure that the CurrentResourceOwner is not about to be released
before
creating a DSM under the CurrentResourceOwner.

The attached fix resolves this issue. However, for a more comprehensive
solution,
I believe the same change should be extended to other parts of the DSA and
DSM
code where CurrentResourceOwner is referenced.

> I have not been able to reproduce this issue. Could you please clarify
which process you ran
pg_get_process_memory_context() on, with the interval of 0.1? Was it a
backend process
created by make installcheck-world, or some other process?

Thank you,
Rahila Syed

---

From: Fujii Masao <[email-masked]>
Date: Thu, 9 Jan 2025 00:45:48 +0900

On 2025/01/08 21:03, Rahila Syed wrote:
> I have not been able to reproduce this issue. Could you please clarify which process you ran
> |pg_get_process_memory_context()| on, with the interval of 0.1?

I used the following query for testing:

=# SELECT count(*) FROM pg_stat_activity, pg_get_process_memory_contexts(pid, false) WHERE pid <> pg_backend_pid();
=# \watch 0.1

> Was it a backend process
> created by |make installcheck-world|, or some other process?

Yes, the target backends were from make installcheck-world.
No other workloads were running.

Regards,

---

From: Rahila Syed <[email-masked]>
Date: Mon, 13 Jan 2025 08:06:10 +0530

Hi Tomas,

On Tue, Jan 7, 2025 at 2:32 AM Tomas Vondra <[email-masked]> wrote:

> I am not sure as of now, I will debug further. Meanwhile, I have addressed
the
review comments. Please find the details and an updated patch below.

> a tree-like structure, so it's unclear if levels are assigned from the
Fixed.

> places that add new stuff).
Done.

> I added the comments above the typedef and changed the name to
PrintDestination.

> +            strncpy(memctx_info[curr_id].type, type, strlen(type));
Got rid of the copy and moved the switch to a separate function.

> Was fixed in your suggestions patch.

> Done the refactoring to move certain parts into separate functions.

> 7) I'm not sure if/why we need to move MemoryContextId to memutils.h.

This is because I am referencing it from both mcxt.c and mcxtfuns.c. I can
consider moving some of the code out of mcxt.c and consolidating
everything related to this patch in mcxtfuncs.c if mcxt.c is intended to
contain only the core memory context logic.

> Fixed.

> Fixed too.

> But this is not just about the signal, it also waits for the results and

Makes sense, edited accordingly.

> I have moved it after the super user check but kept it before some other
checks that lead to WARNING, after looking at how other functions have done
it.

> Edited accordingly.

> Thank you for improving the comments. Added remaining changes as requested.

> it to grow up to 16MB, correct?

Yes.

16MB seems like a lot, certainly enough
> I can try reducing it to 8MB, although it's expected to be only allocated
when needed.

> Done.

> Thank you

> Makes sense, fixed.

> need to detach the DSA and return NULL. Would be better to do a label
Done.

> many processes calling pg_get_process_memory_contexts?

I think this is not possible since if the breaking condition is met, it
means
memstats_dsa_pointer is valid and memctx_info which resides
at mestats_dsa_pointer will contain valid data.  Am I missing something?

Regarding the proc_id == pid check, I have added a comment in the code as
requested.

> Ok.

> Done.

> get_summary before reading it from the shmem.
Fixed. It was not showing up in tests as the result of the bug was some
extra memory allocation
in dsa and some extra computation to populate all the paths in hash table
inspite of
get_summary being true.

> Thank you, applied the 0002 patch and made the changes mentioned in XXX.

Answering some of your questions in the 0002 patch below:

Q. * XXX Also, what if we fill exactly this number of contexts? Won't we
* lose the last entry because it will be overwitten by the summary?
A. We are filling 0 to max_stats - 2 slots by memory context in the loop
   foreach_ptr(MemoryContextData, cur, contexts) in
ProcessGetMemoryContextInterrupt.
   max_stats - 1 is reserved for the summary statistics.

Q.    /* XXX I don't understand why we need to check get_summary here? */
A. get_summary check is there to ensure that the context_id is inserted in
the
hash_table if get_summary is true. If get_summary is true, the loop will
break after the first iteration
and the entire main list of contexts won't be traversed and hence
context_ids won't be inserted.
Hence it is handled separately inside a check for get_summary.

Q. /* XXX What if the memstats_dsa_pointer is not valid? Is it even
possible?
         * If it is, we have garbage in memctx_info. Maybe it should be an
Assert()? */
A . Agreed. Changed it to an assert.

Q.    /*
         * XXX isn't 2 x 1kB for every context a bit too much? Maybe better
to
         * make it variable-length?
         */
A. I don't know how to do this for a variable in shared memory, won't that
mean
allocating from the heap and thus the pointer would become invalid in
another
process?

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Tue, 21 Jan 2025 16:57:14 +0530

Hi Tomas,

 I've tried the pgbench test
> Yes, This occurs when, due to concurrent signals received by a backend,
both signals are processed together, and stats are published only once.
Once the stats are read by the first client that gains access, they are
erased,
causing the second client to wait until timeout.

If we make clients wait for the latest stats, timeouts may occur during
concurrent
operations. To avoid such timeouts, we can retain the previously published
memory
statistics for every backend and avoid waiting for the latest statistics
when the
previous statistics are newer than STALE_STATS_LIMIT. This limit can be
determined
based on the server load and how fast the memory statistics requests are
being
handled by the server.

For example, on a server running make -j 4 installcheck-world while
concurrently
probing client backends for memory statistics using pgbench, accepting
statistics
that were approximately 1 second old helped eliminate timeouts. Conversely,
on an
idle system, waiting for new statistics when the previous ones were older
than 0.1
seconds was sufficient to avoid any timeouts caused by concurrent requests.

PFA an updated and rebased patch that includes the capability to associate
timestamps with statistics. Additionally, I have made some minor fixes and
improved
the indentation.

Currently, I have set STALE_STATS_LIMIT to 0.5 seconds in code. which means
do not
do not wait for newer statistics if previous statistics were published
within the last
5 seconds of current request.

Inshort, there are following options to design the wait for statistics
depending on whether
we expect concurrent requests to a backend for memory statistics to be
common.

1. Always get the latest statistics and timeout if not able to.

This works fine for sequential probing which is going to be the most common
use case.
This can lead to a backend timeouts upto MAX_TRIES * MEMSTATS_WAIT_TIMEOUT.

2. Determine the appropriate STALE_STATS_LIMIT and not wait for the latest
stats if
previous statistics are within that limit .
This will help avoid the timeouts in case of the concurrent requests.

3.  Do what v10 patch on this thread does -

Wait for the latest statistics for up to MEMSTATS_WAIT_TIMEOUT;
otherwise, display the previous statistics, regardless of when they were
published.

Since timeouts are likely to occur only during concurrent requests, the
displayed
statistics are unlikely to be very outdated.
However, in this scenario, we observe the behavior you mentioned, i.e.,
concurrent
backends can get stuck for the duration of MEMSTATS_WAIT_TIMEOUT
(currently 5 seconds as per the current settings).

I am inclined toward the third approach, as concurrent requests are not
expected
to be a common use case for this feature. Moreover, with the second
approach,
determining an appropriate value for STALE_STATS_LIMIT is challenging, as
it
depends on the server's load.

Kindly let me know your preference. I have attached a patch which
implements the
2nd approach for testing, the 3rd approach being implemented in the v10
patch.

Thank you,
Rahila Syed

---

From: Fujii Masao <[email-masked]>
Date: Wed, 22 Jan 2025 01:31:53 +0900

On 2025/01/21 20:27, Rahila Syed wrote:
> Once the stats are read by the first client that gains access, they are erased,
> If we make clients wait for the latest stats, timeouts may occur during concurrent
> operations. To avoid such timeouts, we can retain the previously published memory
> statistics for every backend and avoid waiting for the latest statistics when the
> previous statistics are newer than STALE_STATS_LIMIT. This limit can be determined
> based on the server load and how fast the memory statistics requests are being
> handled by the server.
> 
> For example, on a server running make -j 4 installcheck-world while concurrently
> probing client backends for memory statistics using pgbench, accepting statistics
> that were approximately 1 second old helped eliminate timeouts. Conversely, on an
> idle system, waiting for new statistics when the previous ones were older than 0.1
> timestamps with statistics. Additionally, I have made some minor fixes and improved
> the indentation.
> 
> Currently, I have set STALE_STATS_LIMIT to 0.5 seconds in code. which means do not
> do not wait for newer statistics if previous statistics were published within the last
> Inshort, there are following options to design the wait for statistics depending on whether
> we expect concurrent requests to a backend for memory statistics to be common.
> 
> This works fine for sequential probing which is going to be the most common use case.
> 2. Determine the appropriate STALE_STATS_LIMIT and not wait for the latest stats if
> otherwise, display the previous statistics, regardless of when they were published.
> 
> Since timeouts are likely to occur only during concurrent requests, the displayed
> However, in this scenario, we observe the behavior you mentioned, i.e., concurrent
> I am inclined toward the third approach, as concurrent requests are not expected
> to be a common use case for this feature. Moreover, with the second approach,
> determining an appropriate value for STALE_STATS_LIMIT is challenging, as it

Just idea; as an another option, how about blocking new requests to
the target process (e.g., causing them to fail with an error or
returning NULL with a warning) if a previous request is still pending?
Users can simply retry the request if it fails. IMO failing quickly
seems preferable to getting stuck for a while in cases with concurrent
requests.

Regards,

---

From: Rahila Syed <[email-masked]>
Date: Fri, 24 Jan 2025 19:17:35 +0530

Hi,

> Thank you for the suggestion. I agree that it is better to fail early
and avoid
waiting for a timeout in such cases. I will add a "pending request" tracker
for
this in shared memory. This approach will help prevent sending a concurrent
request if a request for the same backend is still being processed.
IMO, one downside of throwing an error in such cases is that the users
might
wonder if they need to take a corrective action, even though the issue is
actually
going to solve itself and they just need to retry. Therefore, issuing a
warning
or displaying previously updated statistics might be a better alternative
to throwing
an error.

Thank you,
Rahila Syed

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 24 Jan 2025 23:20:50 +0100

On 1/24/25 14:47, Rahila Syed wrote:
> Thank you for the suggestion. I agree that it is better to fail 
> early and avoid waiting for a timeout in such cases. I will add a 
> "pending request" tracker for this in shared memory. This approach 
> will help prevent sending a concurrent request if a request for the
> same backend is still being processed.
> 

AFAIK these failures should be extremely rare - we're only talking about
that because the workload I used for testing is highly concurrent, i.e.
it requests memory context info extremely often. I doubt anyone sane is
going to do that in practice ...

> IMO, one downside of throwing an error in such cases is that the
> users might wonder if they need to take a corrective action, even
> though the issue is actually going to solve itself and they just
> need to retry. Therefore, issuing a warning or displaying previously
> updated statistics might be a better alternative to throwing an
> error.
> 

Wouldn't this be mostly mitigated by adding proper detail/hint to the
error message? Sure, the user can always ignore that (especially when
calling this from a script), but well ... we can only do so much.

All this makes me think about how we shared pgstat data before the shmem
approach was introduced in PG15. Until then the process signaled pgstat
collector, and the collector wrote the statistics into a file, with a
timestamp. And the process used the timestamp to decide if it's fresh
enough ... Wouldn't the same approach work here?

I imagined it would work something like this:

requesting backend:
-------------------
* set request_ts to current timestamp
* signal the target process, to generate memory context info
* wait until the DSA gets filled with stats_ts > request_ts
* return the data, don't erase anything

target backend
--------------
* clear the signal
* generate the statistics
* set stats_ts to current timestamp
* wait all the backends waiting for the stats (through CV)

I see v11 does almost this, except that it accepts somewhat stale data.
But why would that be necessary? I don't think it's needed, and I don't
think we should accept data from before the process sends the signal.

regards

-- 
Tomas Vondra

---

From: Rahila Syed <[email-masked]>
Date: Wed, 29 Jan 2025 18:15:38 +0530

Hi,

> going to do that in practice ...

Yes, that makes sense.

> calling this from a script), but well ... we can only do so much.

 OK.

> I imagined it would work something like this:
That's correct.

> This is done in an attempt to avoid concurrent requests from timing out.
In such cases, data in response to another request is likely to already be
in the
dynamic shared memory. Hence instead of waiting for the latest data and
risking a
timeout, the approach displays available statistics that are newer than a
defined
threshold. Additionally, since we can't distinguish between sequential and
concurrent requests, we accept somewhat stale data for all requests.

I realize this approach has some issues, mainly regarding how to determine
an appropriate threshold value or a limit for old data.

Therefore, I agree that it makes sense to display the data that is
published
after the request is made. If such data can't be published due to
concurrent
requests or other delays, the function should detect this and return as
soon as
possible.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Mon, 3 Feb 2025 18:17:14 +0530

Hi,

> > same backend is still being processed.

Please find attached a patch that adds a request_pending field in
shared memory. This allows us to detect concurrent requests early
and return a WARNING message immediately, avoiding unnecessary
waiting and potential timeouts. This is added in v12-0002* patch.

> requesting backend:

The attached v12-0002* patch implements this. We determine
the latest statistics based on the stats timestamp, if it is greater
than the timestamp when the request was sent, the statistics are
considered up to date and are returned immediately.  Otherwise,
the  client waits for the latest statistics to be published until the
timeout is reached.

With the latest changes, I don't see a dip in tps even when
concurrent requests are run in pgbench script.

 pgbench -n -f monitoring.sql -P 1 postgres -T 60
pgbench (18devel)
progress: 1.0 s, 816.9 tps, lat 1.218 ms stddev 0.317, 0 failed
progress: 2.0 s, 821.9 tps, lat 1.216 ms stddev 0.177, 0 failed
progress: 3.0 s, 817.1 tps, lat 1.224 ms stddev 0.209, 0 failed
progress: 4.0 s, 791.0 tps, lat 1.262 ms stddev 0.292, 0 failed
progress: 5.0 s, 780.8 tps, lat 1.280 ms stddev 0.326, 0 failed
progress: 6.0 s, 675.2 tps, lat 1.482 ms stddev 0.503, 0 failed
progress: 7.0 s, 674.0 tps, lat 1.482 ms stddev 0.387, 0 failed
progress: 8.0 s, 821.0 tps, lat 1.217 ms stddev 0.272, 0 failed
progress: 9.0 s, 903.0 tps, lat 1.108 ms stddev 0.196, 0 failed
progress: 10.0 s, 886.9 tps, lat 1.128 ms stddev 0.160, 0 failed
progress: 11.0 s, 887.1 tps, lat 1.126 ms stddev 0.243, 0 failed
progress: 12.0 s, 871.0 tps, lat 1.147 ms stddev 0.227, 0 failed
progress: 13.0 s, 735.0 tps, lat 1.361 ms stddev 0.329, 0 failed
progress: 14.0 s, 655.9 tps, lat 1.522 ms stddev 0.331, 0 failed
progress: 15.0 s, 674.0 tps, lat 1.484 ms stddev 0.254, 0 failed
progress: 16.0 s, 659.0 tps, lat 1.517 ms stddev 0.289, 0 failed
progress: 17.0 s, 641.0 tps, lat 1.558 ms stddev 0.281, 0 failed
progress: 18.0 s, 707.8 tps, lat 1.412 ms stddev 0.324, 0 failed
progress: 19.0 s, 746.3 tps, lat 1.341 ms stddev 0.219, 0 failed
progress: 20.0 s, 659.9 tps, lat 1.513 ms stddev 0.372, 0 failed
progress: 21.0 s, 651.8 tps, lat 1.533 ms stddev 0.372, 0 failed
WARNING:  cannot process the request at the moment
HINT:  Another request is pending, try again
progress: 22.0 s, 635.2 tps, lat 1.574 ms stddev 0.519, 0 failed
WARNING:  cannot process the request at the moment
HINT:  Another request is pending, try again
progress: 23.0 s, 730.0 tps, lat 1.369 ms stddev 0.408, 0 failed
WARNING:  cannot process the request at the moment
HINT:  Another request is pending, try again
WARNING:  cannot process the request at the moment
HINT:  Another request is pending, try again

where monitoring.sql is as follows:

I have split the patch into 2 patches with v12-0001* consisting of fixes
needed to allow using the MemoryContextStatsInternals for this
feature and
v12-0002* containing all the remaining changes for the feature.

A few outstanding issues are as follows:

1.  Currently one DSA  is created per backend when the first request for
statistics is made and remains for the lifetime of the server.
I think I should add logic to periodically destroy DSAs, when memory
context statistics are not being *actively* queried from the backend,
as determined by the statistics timestamp.
2. The two issues reported by Fujii-san here: [1].
i. I have proposed a fix for the first issue here [2].
ii. I am able to reproduce the second issue. This happens when we try
to query statistics of a backend running infinite_recurse.sql. While I am
working on finding a root-cause, I think it happens due to some memory
being overwritten due to to stack-depth violation, as the issue is not seen
when I reduce the max_stack_depth to 100kb.

[1].
https://www.postgresql.org/message-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3%40oss.nttdata.com
[2].
https://www.postgresql.org/message-id/CAH2L28shr0j3JE5V3CXDFmDH-agTSnh2V8pR23X0UhRMbDQD9Q%40mail.gmail.com

---

From: torikoshia <[email-masked]>
Date: Mon, 10 Feb 2025 21:02:56 +0900

> waiting and potential timeouts. This is added in v12-0002* patch.

Thanks for updating the patch!

The below comments would be a bit too detailed at this stage, but I’d 
like to share the points I noticed.

>  76 +        arguments: PID and a boolean, get_summary. The function 
> can send

Since get_summary is a parameter, should we enclose it in <parameter> 
tags, like <parameter>get_summary</parameter>?

> 387 + * The shared memory buffer has a limited size - it the process 
> has too many
> 388 + * memory contexts,

Should 'it' be 'if'?

> 320  * By default, only superusers are allowed to signal to return the 
> memory
> 321  * contexts because allowing any users to issue this request at an 
> unbounded
> 322  * rate would cause lots of requests to be sent and which can lead 
> to denial of
> 323  * service. Additional roles can be permitted with GRANT.

This comment seems to contradict the following code:

> 360      * Only superusers or users with pg_read_all_stats privileges 
> can view the
> 361      * memory context statistics of another process
> 362      */
> 363     if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_STATS))
> 364         ereport(ERROR,
> 365                 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> 366                  errmsg("memory context statistics privilege 
> error")));

> 485 +   if (memCtxState[procNumber].memstats_dsa_handle == 
> DSA_HANDLE_INVALID)
> 486 +   {
> 487 +
> 488 +       LWLockRelease(&memCtxState[procNumber].lw_lock);

> 505 +   else
> 506 +   {
> 507 +       LWLockRelease(&memCtxState[procNumber].lw_lock);

The LWLockRelease() function appears in both the if and else branches. 
Can we move it outside the conditional block to avoid duplication?

  > 486 +   {
  > 487 +
  > 488 +       LWLockRelease(&memCtxState[procNumber].lw_lock);

The blank line at 487 seems unnecessary. Should we remove it?

> 534         {
> 535             ereport(LOG,
> 536                     (errmsg("Wait for %d process to publish stats 
> timed out, trying again",
> 537                             pid)));
> 538             if (num_retries > MAX_RETRIES)
> 539                 goto end;
> 540             num_retries = num_retries + 1;
> 541         }

If the target process remains unresponsive, the logs will repeatedly 
show:

   LOG:  Wait for xxxx process to publish stats timed out, trying again
   LOG:  Wait for xxxx process to publish stats timed out, trying again
   ...
   LOG:  Wait for xxxx process to publish stats timed out, trying again

However, the final log message is misleading because it does not 
actually try again. Should we adjust the last log message to reflect the 
correct behavior?

> 541         }
> 542
> 543     }

The blank line at 542 seems unnecessary. Should we remove it?

> 874 +   context_id_lookup = 
> hash_create("pg_get_remote_backend_memory_contexts",

Should 'pg_get_remote_backend_memory_contexts' be renamed to 
'pg_get_process_memory_contexts' now?

> 899 +    * Allocate memory in this process's dsa for storing statistics 
> of the the

'the the' is a duplicate.

-- 
Regards,

---

From: Rahila Syed <[email-masked]>
Date: Tue, 18 Feb 2025 18:35:38 +0530

Hi,

> Thanks for sharing the detailed comments. I have incorporated some of them
into the new version of the patch. I will include the rest when I refine and
comment the code further.

Meanwhile, I have fixed the following outstanding issues:

> as determined by the statistics timestamp.

After an offline discussion with Andres and Tomas, I have fixed this to use
only one DSA for all the publishing backends/processes. Each backend
 allocates smaller chunks of memory within the DSA while publishing
statistics.
These chunks are tracked independently by each backend, ensuring that two
publishing backends/processes do not block each other despite using the
same
DSA. This approach eliminates the overhead of creating multiple DSAs,
one for each backend.

I am not destroying the DSA area because it stores the previously published
statistics for each process. This allows the system to display older
statistics
when the latest data cannot be retrieved within a reasonable time.
Only the most recently updated statistics are kept, while all earlier ones
are freed using dsa_free by each backend when they are no longer needed.
.

> when I reduce the max_stack_depth to 100kb.

The second issue is also resolved by using smaller allocations within a
DSA.
Previously, it occurred because a few statically allocated strings were
placed
within a single large chunk of DSA allocation. I have changed this to use
dynamically allocated chunks with dsa_allocate0 within the same DSA.

Please find attached updated and rebased patches.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Thu, 20 Feb 2025 18:56:49 +0530

Hi,

Please find attached the updated patches after some cleanup and test
fixes.

Thank you,
Rahila Syed

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 21 Feb 2025 16:01:00 +0100

On 2/20/25 14:26, Rahila Syed wrote:
> On Tue, Feb 18, 2025 at 6:35 PM Rahila Syed <[email-masked]
> <mailto:[email-masked]>> wrote:
> 
>     Hi,
> 
> 
>     publishing backends/processes do not block each other despite using
>     the same

I think something is not quite right, because if I try running a simple
pgbench script that does pg_get_process_memory_contexts() on PIDs of
random postgres process (just like in the past), I immediately get this:

pgbench: error: client 28 script 0 aborted in command 0 query 0: ERROR:
can't attach the same segment more than once
pgbench: error: client 10 script 0 aborted in command 0 query 0: ERROR:
can't attach the same segment more than once
pgbench: error: client 5 script 0 aborted in command 0 query 0: ERROR:
can't attach the same segment more than once
pgbench: error: client 8 script 0 aborted in command 0 query 0: ERROR:
can't attach the same segment more than once
...

Perhaps the backends need to synchronize creation of the DSA?

>     The second issue is also resolved by using smaller allocations
>     within a DSA.
>     Previously, it occurred because a few statically allocated strings
>     were placed

Sounds good. Do you have any measurements how much this reduced the size
of the entries written to the DSA? How many entries will fit into 1MB of
shared memory?

regards

-- 
Tomas Vondra

---

From: Rahila Syed <[email-masked]>
Date: Mon, 24 Feb 2025 18:16:45 +0530

> Thank you for testing. This issue occurs when a process that previously
attached
to a DSA area for publishing its own context statistics tries to attach to
it again while
querying statistics from another backend. Previously, I was not detaching
at the end
of publishing the statistics. I have now changed it to detach from the area
after the
statistics are published. The fix is included in the updated patch.

> Perhaps the backends need to synchronize creation of the DSA?

This has been implemented in the patch.

> of the entries written to the DSA? How many entries will fit into 1MB of

The size of the entries has approximately halved after dynamically
allocating the
strings and a datum array.
Also, previously, I was allocating the entire memory for all contexts in
one large chunk
from DSA. I have now separated them into smaller allocations
per context. The integer counters are still allocated at once for all
contexts, but
the size of an allocated chunk will not exceed approximately 128 bytes *
total_num_of_contexts.
Average total number of contexts is in the hundreds.

PFA the updated and rebased patches.

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 28 Feb 2025 16:42:37 +0100

> On 24 Feb 2025, at 13:46, Rahila Syed <[email-masked]> wrote:

> PFA the updated and rebased patches.

Thanks for the rebase, a few mostly superficial comments from a first
read-through.  I'll do some more testing and playing around with it for
functional comments.

+  ...
+  child contexts' statistics, with num_agg_contexts indicating the number
+  of these aggregated child contexts.
The documentation refers to attributes in the return row but the format of that
row isn't displayed which makes following along hard.  I think we should
include a table or a programlisting showing the return data before this
paragraph.

+const char *
+AssignContextType(NodeTag type)
This function doesn't actually assign anything so the name is a bit misleading,
it would be better with ContextTypeToString or something similar.

+ * By default, only superusers or users with PG_READ_ALL_STATS are allowed to
This sentence is really long and should probably be broken up.

s/it/if/

+ * If the publishing backend does not respond before the condition variable
+ * times out, which is set to MEMSTATS_WAIT_TIMEOUT, retry for max_tries
+ * number of times, which is defined by user, before giving up and
+ * returning previously published statistics, if any.
This comment should mention what happens if the process gives up and there is
no previously published stats.

+   int         i;
    ...
+   for (i = 0; i < memCtxState[procNumber].total_stats; i++)
This can be rewritten as "for (int i = 0; .." since we allow C99.

+    * process running and consuming lots of memory, that it might end on its
+    * own first and its memory contexts are not logged is not a problem.
This comment is copy/pasted from pg_log_backend_memory_contexts and while it
mostly still apply it should at least be rewritten to not refer to logging as
this function doesn't do that.

+   ereport(WARNING,
+           (errmsg("PID %d is not a PostgreSQL server process",
No need to add the extra parenthesis around errmsg anymore, so I think new code
should omit those.

+  errhint("Use pg_backend_memory_contexts view instead")));
Super nitpick, but errhints should be complete sentences ending with a period.

+    * statitics have previously been published by the backend. In which case,
s/statitics/statistics/

+    * statitics have previously been published by the backend. In which case,
+    * check if statistics are not older than curr_timestamp, if they are wait
I think the sentence around the time check is needlessly confusing, could it be
rewritten into something like:
    "A valid DSA pointer isn't proof that statistics are available, it can be
    valid due to previously published stats.  Check if the stats are updated by
    comparing the timestamp, if the stats are newer than our previously
    recorded timestamp from before sending the procsignal they must by
    definition be updated."

+   /* Assert for dsa_handle to be valid */
Was this intended to be turned into an Assert call? Else it seems better to remove.

+   if (print_location != PRINT_STATS_NONE)
This warrants a comment stating why it makes sense.

+    * Do not print the statistics if print_to_stderr is PRINT_STATS_NONE,
s/print_to_stderr/print_location/.  Also, do we really need print_to_stderr in
this function at all?  It seems a bit awkward to combine a boolean and a
paramter for a tri-state value when the parameter holds the tri_state already.
For readability I think just checking print_location will be better since the
value will be clear, where print_to_stderr=false is less clear in a tri-state
scenario.

+ * its ancestors to a list, inorder to compute a path.
s/inorder/in order/

+   elog(LOG, "hash table corrupted, can't construct path value");
+   break;
This will return either a NIL list or a partial path, but PublishMemoryContext
doesn't really take into consideration that it might be so.  Is this really
benign to the point that we can blindly go on?  Also, elog(LOG..) is mostly for
tracing or debugging as elog() isn't intended for user facing errors.

+static void
+compute_num_of_contexts(List *contexts, HTAB *context_id_lookup,
+                       int *stats_count, bool get_summary)
This function does a lot than compute the number of contexts so the name seems
a bit misleading.  Perhaps a rename to compute_contexts() or something similar?

+   memctx_info[curr_id].name = dsa_allocate0(area,
+                                             strlen(clipped_ident) + 1);
These calls can use idlen instead of more strlen() calls no?  While there is no
performance benefit, it would increase readability IMHO if the code first
calculates a value, and then use it.

+   strncpy(name,
+           clipped_ident, strlen(clipped_ident));
Since clipped_ident has been nul terminated earlier there is no need to use
strncpy, we can instead use strlcpy and take the target buffer size into
consideration rather than the input string length.

    PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
+   PROCSIG_GET_MEMORY_CONTEXT, /* ask backend to log the memory contexts */
This comment should be different from the LOG_MEMORY_xx one.

+#define MEM_CONTEXT_SHMEM_STATS_SIZE   30
+#define MAX_TYPE_STRING_LENGTH 64
These are unused, from an earlier version of the patch perhaps?

+ * Singe DSA area is created and used by all the processes,
s/Singe/Since/

+typedef struct MemoryContextBackendState
This is only used in mcxtfuncs.c and can be moved there rather than being
exported in the header.

+}          MemoryContextId;
This lacks an entry in the typedefs.list file.

--
Daniel Gustafsson

---

From: Rahila Syed <[email-masked]>
Date: Tue, 4 Mar 2025 12:30:02 +0530

Hi Daniel,

> read-through.
Thank you for your comments.

> I included the sql function call and its output in programlisting format
after the
function description.
Since the description was part of a table, I added this additional
information at the
end of that table.

> Replaced accordingly.

> Was this intended to be turned into an Assert call? Else it seems better

Added an assert and removed the comment.

> I removed the boolean print_to_stderr, the checks are now using
the tri-state enum-print_location.

> I agree that this should be addressed. I added a check for path value
before
storing it in shared memory. If the path is NIL, the path pointer in DSA
will point
to InvalidDsaPointer.
When a client encounters an InvalidDsaPointer it will print NULL in the
path column.
Partial path scenario is unlikely IMO, and I am not sure if it warrants
additional
checks.

> Renamed to compute_contexts_count_and_ids.

> Replaced with the strlcpy calls.

> Removed

Fixed.

> This is only used in mcxtfuncs.c and can be moved there rather than being

This is being used in mcxt.c too in the form of the variable memCtxState.

+}          MemoryContextId;
> Added.

Please find attached the updated patches with the above fixes.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Thu, 13 Mar 2025 19:26:51 +0530

Hi,

Please find attached updated and rebased patches. It has the following
changes

1. To prevent memory leaks, ensure that the latest statistics published by
a process
are freed before it exits. This can be achieved by calling dsa_free in the
before_shmem_exit callback.
2. Add a level column to maintain consistency with the output of
pg_backend_memory_contexts.

Thank you,
Rahila Syed

---

From: Alexander Korotkov <[email-masked]>
Date: Sat, 15 Mar 2025 10:40:39 +0200

Hi, Rahila!

On Thu, Mar 13, 2025 at 3:57 PM Rahila Syed <[email-masked]> wrote:
>
> Please find attached updated and rebased patches. It has the following changes
>
> 1. To prevent memory leaks, ensure that the latest statistics published by a process

Thank you for your work on this subject.

v17-0001-Preparatory-changes-for-reporting-memory-context-sta.patch

It looks like we're increasing *num_contexts twice per child memory
context.  First, it gets increased with a recursive
MemoryContextStatsInternal() call, then by adding an ichild.  I might
be wrong, but I think these calculations at least deserve more
comments.

v17-0002-Function-to-report-memory-context-statistics.patch

+   if (procNumber == MyProcNumber)
+   {
+       ereport(WARNING,
+               errmsg("cannot return statistics for local backend"),
+               errhint("Use pg_backend_memory_contexts view instead."));
+       PG_RETURN_NULL();
+   }

Is it worth it to keep this restriction?  Can we fetch info about
local memory context for the sake of generality?

I know there have been discussions in the thread before, but the
mechanism of publishing memory context stats via DSA looks quite
complicated.  Also, the user probably intends to inspect memory
contexts when there is not a huge amount of free memory.  So, failure
is probable on DSA allocation.  Could we do simpler?  For instance,
allocate some amount of static shared memory and use it as a message
queue between processes.  As a heavy load is not supposed to be here,
I think one queue would be enough.

------
Regards,
Alexander Korotkov
Supabase

---

From: Rahila Syed <[email-masked]>
Date: Mon, 17 Mar 2025 13:22:41 +0530

Hi Alexander,

> be wrong, but I think these calculations at least deserve more

I believe that's not the case. The recursive calls only work for children
encountered up to max_level and less than max_children per context.
The rest of the children are handled using MemoryContextTraverseNext,
without recursive calls. Thus, num_contexts is incremented for those
children separately from the recursive call counter.

I will add more comments around this.

> local memory context for the sake of generality?
I think that could be done, but using pg_backend_memory_context would
be more efficient in this case.

> I think one queue would be enough.
There could be other uses for such a function, such as a monitoring
dashboard
that periodically queries all running backends for memory statistics. If we
use a
single queue shared between all the backends, they will need to wait for
the queue
to become available before sharing their statistics, leading to processing
delays at
the publishing backend.

Even with separate queues for each backend or without expecting concurrent
use,
publishing statistics could be delayed if a message queue is full. This is
because a
backend needs to wait for a client process to consume existing messages or
statistics before publishing more.
If a client process exits without consuming messages, the publishing
backend will
experience timeouts when trying to publish stats. This will impact backend
performance
as statistics are published during CHECK_FOR_INTERRUPTS.

In the current implementation, the backend publishes all the statistics in
one go
without waiting for clients to read any statistics.

In addition, allocating complete message queues in static shared memory can
be
expensive, especially since these static structures need to be created even
if memory
context statistics are never queried.
On the contrary, a dsa is created for the feature whenever statistics are
first queried.
We are not preallocating shared memory for this feature, except for small
structures
to store the dsa_handle and dsa_pointers for each backend.

Thank you,
Rahila Syed

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 17 Mar 2025 14:06:50 +0530

> be more efficient in this case.

I have raised a similar concern before. Having two separate functions
one for local backend and other for remote is going to be confusing.
We should have one function doing both and renamed appropriately.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Rahila Syed <[email-masked]>
Date: Thu, 20 Mar 2025 13:09:45 +0530

Hi,

> We should have one function doing both and renamed appropriately.
This is a separate concern from what has been raised by Alexander.
He has suggested removing the restriction and fetching local backend
statistics also
with the proposed function.
I've removed this restriction in the latest version of the patch. Now, the
proposed
function can be used to fetch local backend statistics too.

Regarding your suggestion on merging these functions, although they both
report memory
context statistics, they differ in how they fetch these statistics—locally
versus from dynamic
shared memory. Additionally, the function signatures are different: the
proposed function
takes three arguments (pid, get_summary, and num_tries), whereas
pg_get_backend_memory_contexts does not take any arguments. Therefore, I
believe
these functions can remain separate as long as we document their usages
correctly.

Please find attached rebased and updated patches. I have added some more
comments and
fixed an issue caused due to registering before_shmem_exit callback from
interrupt processing
routine. To address this issue, I am registering this callback in the
InitProcess() function.

This happened because interrupt processing could be triggered from a
PG_ENSURE_ERROR_CLEANUP block. This block operates under the assumption
that
the before_shmem_exit callback registered at the beginning of the block,
will be the last one
in the registered callback list at the end of the block, which would not be
the case if I register
before_shmem_exit callback in the interrupt handling routine.

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 25 Mar 2025 15:14:08 +0100

> On 20 Mar 2025, at 08:39, Rahila Syed <[email-masked]> wrote:

Thanks for the new version, I believe this will be a welcome tool in the
debugging toolbox.

I took a cleanup pass over the docs with among others the below changes:
  * You had broken the text in paragraphs, but without <para/> tags they are
    rendered as a single blob of text so added that.
  * Removed the "(PID)" explanation as process id is used elsewhere on the same
    page already without explanation.
  * Added <productname/> markup on PostgreSQL
  * Added <literal/> markup on paramater values
  * Switched the example query output to use \x
  * Added a note on when pg_backend_memory_contexts is a better choice
The paragraphs need some re-indenting to avoid too long lines, but I opted out
of doing so here to make reviewing the changes easier.

A few comments on the code (all comments are performed in 0003 attached here
which also has smaller cleanups wrt indentation, code style etc):

+#include <math.h>
I don't think we need this, maybe it was from an earlier version of the patch?

+MEM_CTX_PUBLISH    "Waiting for backend to publish memory information."
I wonder if this should really be "process" and not backend?

+       default:
+           context_type = "???";
+           break;
In ContextTypeToString() I'm having second thoughts about this, there shouldn't
be any legitimate use-case of passing a nodetag this function which would fail
MemoryContextIsValid().  I wonder if we aren't helping callers more by erroring
out rather than silently returning an unknown?  I haven't changed this but
maybe we should to set the API right from the start?

+ * if the process has more memory contexts than that can fit in the allocated
s/than that can/than what can/?

+   errmsg("memory context statistics privilege error"));
Similar checks elsewhere in the tree mostly use "permission denied to .." so I
think we should adopt that here as well.

+       LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
+       msecs =
+           TimestampDifferenceMilliseconds(curr_timestamp,
+                                           memCtxState[procNumber].stats_timestamp);
Since we only want to consider the stats if they are from the current process,
we can delay checking the time difference until after we've checked the pid and
thus reduce the amount of time we hold the lock in the error case.

+       /*
+        * Recheck the state of the backend before sleeping on the condition
+        * variable
+        */
+       proc = BackendPidGetProc(pid);
Here we are really rechecking that the process is still alive, but I wonder if
we should take the opportunity to ensure that the type is what we expect it to
be?  If the pid has moved from being a backend to an aux proc or vice versa we
really don't want to go on.

+     ereport(WARNING,
+             errmsg("PID %d is not a PostgreSQL server process",
+                    pid));
I wonder if we should differentiate between the warnings?  When we hit this in
the loop the errmsg is describing a slightly different case.  I did leave it
for now, but it's food for thought if we should perhaps reword this one.

+       ereport(LOG,
+               errmsg("Wait for %d process to publish stats timed out, trying again",
+                      pid));
This should probably by DEBUG1, in a congested cluster it might cause a fair
bit of logging which isn't really helping the user.  Also, nitpick, errmsg
starts with a lowercase letter.

+static Size
+MemCtxShmemSize(void)
We don't really need this function anymore and keeping it separate we risk it
going out of sync with the matching calcuation in MemCtxBackendShmemInit, so I
think we should condense into one.

    else
    {
+       Assert(print_location == PRINT_STATS_NONE);
Rather than an if-then-else and an assert we can use a switch statement without
a default, this way we'll automatically get a warning if a value is missed.

+       ereport(LOG,
+               errmsg("hash table corrupted, can't construct path value"));
I know you switched from elog(LOG..  to ereport(LOG..  but I still think a LOG
entry stating corruption isn't helpful, it's not actionable for the user.
Given that it's a case that shouldn't happen I wonder if we should downgrade it
to an Assert(false) and potentially a DEBUG1?

--
Daniel Gustafsson

---

From: Rahila Syed <[email-masked]>
Date: Wed, 26 Mar 2025 16:04:17 +0530

Hi Daniel,

Thank you for your review.

I have incorporated all your changes in v20 patches and ensured that the
review comments
corresponding to 0001 patch are included in that patch and not in 0002.

> maybe we should to set the API right from the start?

I cannot think of any legitimate scenario where the context type would be
unknown.
However, if we were to throw an error, it would prevent us from reporting
any memory
usage information when the context type is unidentified. Perhaps, it would
be more
informative and less restrictive to label it as "Unrecognized" or "Unknown."
I wonder if this was the reasoning behind doing it when it was added with
the
pg_backend_memory_contexts() function.

> really don't want to go on.
The reasoning makes sense to me. For periodic monitoring of all processes,
any PID that reincarnates into a different type could be queried in
subsequent
function calls. Regarding targeted monitoring of a specific process, such a
reincarnated
process would exhibit a completely different memory consumption,
likely not aligning with the user's original intent behind requesting the
statistics.

> for now, but it's food for thought if we should perhaps reword this one.
Changed it to  "PID %d is no longer the same PostgreSQL server process".

> How about changing it to ERROR, in accordance with current occurrences of
the
same message? I did it in the attached version, however I am open to
changing
it to an Assert(false) and DEBUG1.

Apart from the above, I made the following improvements.

1. Eliminated the unnecessary creation of an extra memory context before
calling hash_create.
The hash_create function already generates a memory context containing the
hash table,
enabling easy memory deallocation by simply deleting the context via
hash_destroy.
Therefore, the patch relies on hash_destroy for memory management instead
of manual freeing.

2. Optimized memory usage by storing the path as an array of integers
rather than as an array of
Datums.
This approach conserves DSA memory allocated for storing this information.

3. Miscellaneous comment cleanups and introduced macros to simplify
calculations.

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 2 Apr 2025 23:44:25 +0200

> On 26 Mar 2025, at 11:34, Rahila Syed <[email-masked]> wrote:

> How about changing it to ERROR, in accordance with current occurrences of the
> same message? I did it in the attached version, however I am open to changing

In the attached I moved it to an elog() as it's an internal error, and spending
translation effort on it seems fruitless.

> 1. Eliminated the unnecessary creation of an extra memory context before calling hash_create. 
> The hash_create function already generates a memory context containing the hash table, 
> enabling easy memory deallocation by simply deleting the context via hash_destroy. 
> Therefore, the patch relies on hash_destroy for memory management instead of manual freeing.

Nice

> 2. Optimized memory usage by storing the path as an array of integers rather than as an array of 
> Datums. 

Ah yes, much better.

The attached v21 has a few improvements:

* The function documentation didn't specify the return type, only the fact that
it's setof record.  I've added all output columns.

* Some general cleaups of the docs with better markup, improved xref linking
and various rewording.

* Comment cleanups and language alignment

* Added a missing_ok parameter to ContextTypeToString().  While all callers are
fine with unknown context types, if we introduce an API for this it seems
prudent to not place that burden on callers but to take it on in the function.

* Renamed get_summary to just summary, and num_of_tries to retries which feels
more in line with the naming convention in other functions

* Deferred calling InitMaterializedSRF() until after the PID has been checked
for validity.

* Pulled back the timeout to 500msec from 1 second.  In running congested
pgbench simulations I saw better performance and improved results in getting stats.

* Replaced strncpy with strlcpy and consistently used idlen to keep all length
calculations equal.

* Fixed misspelled param name in pg_proc.dat

* Pulled back maximum memory usage from 8Mb to 1Mb.  8Mb for the duration of a
process (once allocated) is a lot for a niche feature and I while I'm still not
sure 1Mb is the right value I think from experimentation that it's closer.

I think this version is close to a committable state, will spend a little more
time testing, polishing and rewriting the commit message.  I will also play
around with placement within the memory context code files to keep it from
making backpatch issues.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 5 Apr 2025 21:29:21 +0200

> On 2 Apr 2025, at 23:44, Daniel Gustafsson <[email-masked]> wrote:

> around with placement within the memory context code files to keep it from

After a bit more polish I landed with the attached, which I most likely will go
ahead with after another round in CI.

--
Daniel Gustafsson

---

From: Rahila Syed <[email-masked]>
Date: Mon, 7 Apr 2025 12:21:43 +0530

Hi Daniel,

> ahead with after another round in CI.

Thank you for refining the code. The changes look good to me.
Regression tests ran smoothly in parallel with the memory monitoring
function,
pgbench results with the following custom script also shows good
performance.
```
SELECT * FROM pg_get_process_memory_contexts(
  (SELECT pid FROM pg_stat_activity
    ORDER BY random() LIMIT 1)
  , false, 5);
```

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 7 Apr 2025 15:41:37 +0200

Following up on some off-list comments, attached is a v26 with a few small last
changes:

  * Improved documentation (docs and comments)
  * Fixed up Shmem sizing and init
  * Delayed registering to the shmem cleanup to get it earlier in cleanup
  * Renamed a few datastructures to improve readability
  * Various bits of polish

I think this function can be a valuable debugging aid going forward.

--
Daniel Gustafsson