Subject: pgsql: Add function to get memory context stats for processes

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 08 Apr 2025 09:10:49 +0000

Add function to get memory context stats for processes

This adds a function for retrieving memory context statistics
and information from backends as well as auxiliary processes.
The intended usecase is cluster debugging when under memory
pressure or unanticipated memory usage characteristics.

When calling the function it sends a signal to the specified
process to submit statistics regarding its memory contexts
into dynamic shared memory.  Each memory context is returned
in detail, followed by a cumulative total in case the number
of contexts exceed the max allocated amount of shared memory.
Each process is limited to use at most 1Mb memory for this.

A summary can also be explicitly requested by the user, this
will return the TopMemoryContext and a cumulative total of
all lower contexts.

In order to not block on busy processes the caller specifies
the number of seconds during which to retry before timing out.
In the case where no statistics are published within the set
timeout,  the last known statistics are returned, or NULL if
no previously published statistics exist.  This allows dash-
board type queries to continually publish even if the target
process is temporarily congested.  Context records contain a
timestamp to indicate when they were submitted.

Author: Rahila Syed <[email-masked]>
Reviewed-by: Daniel Gustafsson <[email-masked]>
Reviewed-by: Andres Freund <[email-masked]>
Reviewed-by: Tomas Vondra <[email-masked]>
Reviewed-by: Atsushi Torikoshi <[email-masked]>
Reviewed-by: Fujii Masao <[email-masked]>
Reviewed-by: Alexander Korotkov <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/042a66291b04f473cbc72f95f07438abd75ae3a9

Modified Files
--------------
doc/src/sgml/func.sgml                          | 172 +++++++
src/backend/catalog/system_views.sql            |   5 +
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                  |   3 +
src/backend/storage/ipc/procsignal.c            |   3 +
src/backend/storage/lmgr/lwlock.c               |   2 +
src/backend/storage/lmgr/proc.c                 |   1 +
src/backend/tcop/postgres.c                     |   3 +
src/backend/utils/activity/wait_event_names.txt |   1 +
src/backend/utils/adt/mcxtfuncs.c               | 426 ++++++++++++++--
src/backend/utils/init/globals.c                |   1 +
src/backend/utils/init/postinit.c               |   7 +
src/backend/utils/mmgr/mcxt.c                   | 645 +++++++++++++++++++++++-
src/include/catalog/pg_proc.dat                 |  10 +
src/include/miscadmin.h                         |   1 +
src/include/storage/lwlock.h                    |   2 +
src/include/storage/procsignal.h                |   1 +
src/include/utils/memutils.h                    |  82 +++
src/test/regress/expected/sysviews.out          |  19 +
src/test/regress/sql/sysviews.sql               |  18 +
src/tools/pgindent/typedefs.list                |   4 +
26 files changed, 1385 insertions(+), 45 deletions(-)

---

From: Laurenz Albe <[email-masked]>
Date: Thu, 10 Apr 2025 13:42:02 +0200

On Tue, 2025-04-08 at 09:10 +0000, Daniel Gustafsson wrote:
> [from the patch:]
> diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
> index 016dfd9b3f6..cfe14631445 100644
> --- a/src/include/storage/procsignal.h
> +++ b/src/include/storage/procsignal.h
> [...]
> +extern dsa_area *area;

This commit causes problems for PostGIS, because the name "area" collides
with a PostGIS object:

postgis_legacy.c:58:28: error: ‘area’ redeclared as different kind of symbol
   58 | POSTGIS_DEPRECATE("3.0.0", area)
      |                            ^~~~
postgis_legacy.c:40:15: note: in definition of macro ‘POSTGIS_DEPRECATE’
   40 |         Datum funcname(PG_FUNCTION_ARGS); \
      |               ^~~~~~~~
In file included from ../libpgcommon/lwgeom_pg.h:24,
                 from postgis_legacy.c:37:
/home/laurenz/pg/include/postgresql/server/utils/memutils.h:403:18: note: previous declaration of ‘area’ with type ‘dsa_area *’
  403 | extern dsa_area *area;
      |                  ^~~~

Now one can take the position that PostGIS as dependent library hs to
adapt, but I think "area" is too generic a name.  Could you envision
renaming the global variable to something like "shm_area"?

Attached is a patch for this change.
I am not wedded to the name at all, it was just the first thing that
popped into my head.

Yours,
Laurenz Albe
From 9a207d76d4b1bbe5904bf346c6621324a9dd7359 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <[email-masked]>
Date: Thu, 10 Apr 2025 13:38:10 +0200
Subject: [PATCH v1] Rename a global variable to avoid name collisions

"area", introduced by 042a66291b04, is a very generic name, and
indeed it collides with an object defined by PostGIS.
While it is arguably the job of the dependent library to avoid name
collisions, using a generic name like that is just an invitation
for problems of that kind.

Fix by renaming the variable to "shm_area".
---
 src/backend/utils/adt/mcxtfuncs.c | 14 +++++------
 src/backend/utils/mmgr/mcxt.c     | 40 +++++++++++++++----------------
 src/include/utils/memutils.h      |  2 +-
 3 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 3ede88e5036..154ea38fdc2 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -533,21 +533,21 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 	 */
 	Assert(memCxtArea->memstats_dsa_handle != DSA_HANDLE_INVALID);
 	/* Attach to the dsa area if we have not already done so */
-	if (area == NULL)
+	if (shm_area == NULL)
 	{
 		MemoryContext oldcontext = CurrentMemoryContext;
 
 		MemoryContextSwitchTo(TopMemoryContext);
-		area = dsa_attach(memCxtArea->memstats_dsa_handle);
+		shm_area = dsa_attach(memCxtArea->memstats_dsa_handle);
 		MemoryContextSwitchTo(oldcontext);
-		dsa_pin_mapping(area);
+		dsa_pin_mapping(shm_area);
 	}
 
 	/*
 	 * Backend has finished publishing the stats, project them.
 	 */
 	memcxt_info = (MemoryStatsEntry *)
-		dsa_get_address(area, memCxtState[procNumber].memstats_dsa_pointer);
+		dsa_get_address(shm_area, memCxtState[procNumber].memstats_dsa_pointer);
 
 #define PG_GET_PROCESS_MEMORY_CONTEXTS_COLS	12
 	for (int i = 0; i < memCxtState[procNumber].total_stats; i++)
@@ -566,7 +566,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 
 		if (DsaPointerIsValid(memcxt_info[i].name))
 		{
-			name = (char *) dsa_get_address(area, memcxt_info[i].name);
+			name = (char *) dsa_get_address(shm_area, memcxt_info[i].name);
 			values[0] = CStringGetTextDatum(name);
 		}
 		else
@@ -574,7 +574,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 
 		if (DsaPointerIsValid(memcxt_info[i].ident))
 		{
-			ident = (char *) dsa_get_address(area, memcxt_info[i].ident);
+			ident = (char *) dsa_get_address(shm_area, memcxt_info[i].ident);
 			values[1] = CStringGetTextDatum(ident);
 		}
 		else
@@ -586,7 +586,7 @@ pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
 		path_datum = (Datum *) palloc(path_length * sizeof(Datum));
 		if (DsaPointerIsValid(memcxt_info[i].path))
 		{
-			path_int = (int *) dsa_get_address(area, memcxt_info[i].path);
+			path_int = (int *) dsa_get_address(shm_area, memcxt_info[i].path);
 			for (int j = 0; j < path_length; j++)
 				path_datum[j] = Int32GetDatum(path_int[j]);
 			path_array = construct_array_builtin(path_datum, path_length, INT4OID);
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index cf4e22bf1cc..f32ed7967d9 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -172,7 +172,7 @@ MemoryContext CurTransactionContext = NULL;
 
 /* This is a transient link to the active portal's memory context: */
 MemoryContext PortalContext = NULL;
-dsa_area   *area = NULL;
+dsa_area   *shm_area = NULL;
 
 static void MemoryContextDeleteOnly(MemoryContext context);
 static void MemoryContextCallResetCallbacks(MemoryContext context);
@@ -1499,19 +1499,19 @@ ProcessGetMemoryContextInterrupt(void)
 
 		MemoryContextSwitchTo(TopMemoryContext);
 
-		area = dsa_create(memCxtArea->lw_lock.tranche);
+		shm_area = dsa_create(memCxtArea->lw_lock.tranche);
 
-		handle = dsa_get_handle(area);
+		handle = dsa_get_handle(shm_area);
 		MemoryContextSwitchTo(oldcontext);
 
-		dsa_pin_mapping(area);
+		dsa_pin_mapping(shm_area);
 
 		/*
 		 * Pin the DSA area, this is to make sure the area remains attachable
 		 * even if current backend exits. This is done so that the statistics
 		 * are published even if the process exits while a client is waiting.
 		 */
-		dsa_pin(area);
+		dsa_pin(shm_area);
 
 		/* Set the handle in shared memory */
 		memCxtArea->memstats_dsa_handle = handle;
@@ -1521,14 +1521,14 @@ ProcessGetMemoryContextInterrupt(void)
 	 * If DSA exists, created by another process publishing statistics, attach
 	 * to it.
 	 */
-	else if (area == NULL)
+	else if (shm_area == NULL)
 	{
 		MemoryContext oldcontext = CurrentMemoryContext;
 
 		MemoryContextSwitchTo(TopMemoryContext);
-		area = dsa_attach(memCxtArea->memstats_dsa_handle);
+		shm_area = dsa_attach(memCxtArea->memstats_dsa_handle);
 		MemoryContextSwitchTo(oldcontext);
-		dsa_pin_mapping(area);
+		dsa_pin_mapping(shm_area);
 	}
 	LWLockRelease(&memCxtArea->lw_lock);
 
@@ -1545,7 +1545,7 @@ ProcessGetMemoryContextInterrupt(void)
 		 * Free any previous allocations, free the name, ident and path
 		 * pointers before freeing the pointer that contains them.
 		 */
-		free_memorycontextstate_dsa(area, memCxtState[idx].total_stats,
+		free_memorycontextstate_dsa(shm_area, memCxtState[idx].total_stats,
 									memCxtState[idx].memstats_dsa_pointer);
 	}
 
@@ -1556,10 +1556,10 @@ ProcessGetMemoryContextInterrupt(void)
 	 */
 	memCxtState[idx].total_stats = stats_num;
 	memCxtState[idx].memstats_dsa_pointer =
-		dsa_allocate0(area, stats_num * sizeof(MemoryStatsEntry));
+		dsa_allocate0(shm_area, stats_num * sizeof(MemoryStatsEntry));
 
 	meminfo = (MemoryStatsEntry *)
-		dsa_get_address(area, memCxtState[idx].memstats_dsa_pointer);
+		dsa_get_address(shm_area, memCxtState[idx].memstats_dsa_pointer);
 
 	if (summary)
 	{
@@ -1572,7 +1572,7 @@ ProcessGetMemoryContextInterrupt(void)
 											 &stat, true);
 		path = lcons_int(1, path);
 		PublishMemoryContext(meminfo, cxt_id, TopMemoryContext, path, stat,
-							 1, area, 100);
+							 1, shm_area, 100);
 		cxt_id = cxt_id + 1;
 
 		/*
@@ -1602,7 +1602,7 @@ ProcessGetMemoryContextInterrupt(void)
 			 */
 			memCxtState[idx].total_stats = cxt_id++;
 			PublishMemoryContext(meminfo, cxt_id, c, path,
-								 grand_totals, num_contexts, area, 100);
+								 grand_totals, num_contexts, shm_area, 100);
 		}
 		memCxtState[idx].total_stats = cxt_id;
 
@@ -1632,7 +1632,7 @@ ProcessGetMemoryContextInterrupt(void)
 		if (context_id < (max_stats - 1) || stats_count <= max_stats)
 		{
 			/* Copy statistics to DSA memory */
-			PublishMemoryContext(meminfo, context_id, cur, path, stat, 1, area, 100);
+			PublishMemoryContext(meminfo, context_id, cur, path, stat, 1, shm_area, 100);
 		}
 		else
 		{
@@ -1657,8 +1657,8 @@ ProcessGetMemoryContextInterrupt(void)
 			int			namelen = strlen("Remaining Totals");
 
 			num_individual_stats = context_id + 1;
-			meminfo[max_stats - 1].name = dsa_allocate(area, namelen + 1);
-			nameptr = dsa_get_address(area, meminfo[max_stats - 1].name);
+			meminfo[max_stats - 1].name = dsa_allocate(shm_area, namelen + 1);
+			nameptr = dsa_get_address(shm_area, meminfo[max_stats - 1].name);
 			strncpy(nameptr, "Remaining Totals", namelen);
 			meminfo[max_stats - 1].ident = InvalidDsaPointer;
 			meminfo[max_stats - 1].path = InvalidDsaPointer;
@@ -1921,18 +1921,18 @@ AtProcExit_memstats_cleanup(int code, Datum arg)
 	}
 
 	/* If the dsa mapping could not be found, attach to the area */
-	if (area == NULL)
-		area = dsa_attach(memCxtArea->memstats_dsa_handle);
+	if (shm_area == NULL)
+		shm_area = dsa_attach(memCxtArea->memstats_dsa_handle);
 
 	/*
 	 * Free the memory context statistics, free the name, ident and path
 	 * pointers before freeing the pointer that contains these pointers and
 	 * integer statistics.
 	 */
-	free_memorycontextstate_dsa(area, memCxtState[idx].total_stats,
+	free_memorycontextstate_dsa(shm_area, memCxtState[idx].total_stats,
 								memCxtState[idx].memstats_dsa_pointer);
 
-	dsa_detach(area);
+	dsa_detach(shm_area);
 	LWLockRelease(&memCxtState[idx].lw_lock);
 }
 
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index d328270fafc..79fb4302139 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -400,5 +400,5 @@ extern void HandleGetMemoryContextInterrupt(void);
 extern Size MemoryContextReportingShmemSize(void);
 extern void MemoryContextReportingShmemInit(void);
 extern void AtProcExit_memstats_cleanup(int code, Datum arg);
-extern dsa_area *area;
+extern dsa_area *shm_area;
 #endif							/* MEMUTILS_H */
-- 
2.49.0

---

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 10 Apr 2025 14:48:11 +0300

Hi,

> I am not wedded to the name at all, it was just the first thing that

I agree that the name is too generic for an exported symbol.

-- 
Best regards,
Aleksander Alekseev

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 10 Apr 2025 14:46:35 +0200

> On 10 Apr 2025, at 13:42, Laurenz Albe <[email-masked]> wrote:

> This commit causes problems for PostGIS, because the name "area" collides

Thanks for the report, I've already posted a patch [0] and will push that
shortly.

--
Daniel Gustafsson

[0] https://postgr.es/m/[email-masked]

---

From: Andres Freund <[email-masked]>
Date: Thu, 10 Apr 2025 16:05:42 -0400

Hi,

On 2025-04-10 09:31:00 -0400, Robert Haas wrote:
> On Tue, Apr 8, 2025 at 5:10 AM Daniel Gustafsson
> <[email-masked]> wrote:
> Apologies if this has already been discussed, but what is the argument
> that it is safe to do everything in ProcessGetMemoryContextInterrupt()
> at an arbitrary CHECK_FOR_INTERRUPTS() call? We have
> CHECK_FOR_INTERRUPTS() calls in some quite low-level places, such as
> walkdir() and copydir(). I don't think there's any guarantee that it's
> safe to perform DSA operations at an arbitrary place where
> CHECK_FOR_INTERRUPTS() is called, and I'm not even quite sure that
> it's safe to assume that the local memory-context tree is in a
> consistent state when CHECK_FOR_INTERRUPTS() is called.

I don't know of existing discussion, but it seems rather fundamental to me -
if either DSA or memory contexts could be inconsistent at a CFI(), how could
it possibly be safe to interrupt at that point? After all, after an error you
need to be able to reset the memory contexts / release memory in a
dsa/dshash/whatnot? Memory context reset requires walking over the allocations
made in the context, similar releasing a dsa?

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 14 Apr 2025 10:13:27 -0400

Hi,

On 2025-04-14 10:03:28 -0400, Robert Haas wrote:
> On Thu, Apr 10, 2025 at 4:05 PM Andres Freund <[email-masked]> wrote:
> I think it would be a bit surprising if somebody put a
> CHECK_FOR_INTERRUPTS() inside aset.c or similar, but I don't see a
> reason why we couldn't end up with one reachable via the DSA code. DSA
> calls DSM which depending on dynamic_shared_memory_type might involve
> filesystem operations. That's a fairly large amount of code. I admit I
> have no particular theory about how CFI could be reachable from there
> today, but even if it definitely isn't, I don't see why someone would
> hesitate to add one in the future.

There very well could be a CFI - but it better be somewhere where the
in-memory state is consistent. Otherwise an error inside raised in the CFI
would lead the in-memory state inconsistent which then would cause problems
when cleaning up the dsa during resowner release or process exit.

What am I missing here?

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Tue, 22 Apr 2025 12:58:28 -0400

Hi,

On 2025-04-17 10:42:45 -0400, Robert Haas wrote:
> On Tue, Apr 15, 2025 at 6:11 AM Andres Freund <[email-masked]> wrote:
> I think maybe you're only thinking about gathering the data. What
> about publishing it? If the DSA code were interrupted at a CFI and the
> interrupting code went and tried to perform a DSA allocation to store
> the resulting data and then returned to the interrupted DSA operation,
> would you expect the code to cope with that?

I would expect the DSA code to not call CFI() in such a place - afaict nearly
all such cases would also not be safe against errors being raised in the CFI()
and then later again allocating memory from the DSA (or resetting
it). Similarly, aset.c better not accept interrupts in the middle of, e.g.,
AllocSetAllocFromNewBlock(), since we'd loose track of the allocation.

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 30 Apr 2025 22:29:33 +0200

> On 30 Apr 2025, at 22:17, Robert Haas <[email-masked]> wrote:

> I kind of doubt whether that's the only problem here, but it's *a* problem.

This is indeed exposing a pre-existing issue, which was reported in [0] and a
patch fixing it has been submitted.  I have been testing and reworking the
patch slightly but due to $life and $work events have yet to have time to push
it.

--
Daniel Gustafsson

[0] [email-masked]