pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Add function to get memory context stats for processes
18+ messages / 8 participants
[nested] [flat]
Commits from this thread:
2025-04-10 55ef7abf88 Rename global variable backing DSA area
* pgsql: Add function to get memory context stats for processes
@ 2025-04-08 09:10 Daniel Gustafsson <dgustafsson@postgresql.org>
0 siblings, 2 replies; 18+ messages in thread
From: Daniel Gustafsson @ 2025-04-08 09:10 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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 <rahilasyed90@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Reviewed-by: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Reviewed-by: Fujii Masao <masao.fujii@oss.nttdata.com>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Discussion: https://postgr.es/m/CAH2L28v8mc9HDt8QoSJ8TRmKau_8FM_HKS41NeO9-6ZAkuZKXw@mail.gmail.com
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(-)
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-10 11:42 Laurenz Albe <laurenz.albe@cybertec.at>
parent: Daniel Gustafsson <dgustafsson@postgresql.org>
1 sibling, 2 replies; 18+ messages in thread
From: Laurenz Albe @ 2025-04-10 11:42 UTC (permalink / raw)
To: Daniel Gustafsson <dgustafsson@postgresql.org>; pgsql-committers@lists.postgresql.org
On Tue, 2025-04-08 at 09:10 +0000, Daniel Gustafsson wrote:
> 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.
>
> Discussion: https://postgr.es/m/CAH2L28v8mc9HDt8QoSJ8TRmKau_8FM_HKS41NeO9-6ZAkuZKXw@mail.gmail.com
>
> Details
> -------
> https://git.postgresql.org/pg/commitdiff/042a66291b04f473cbc72f95f07438abd75ae3a9
>
> [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
Attachments:
v1-0001-Rename-a-global-variable-to-avoid-name-collisions.patch (text/x-patch, 8.4K) download
From 9a207d76d4b1bbe5904bf346c6621324a9dd7359 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.albe@cybertec.at>
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
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-10 11:48 Aleksander Alekseev <aleksander@timescale.com>
parent: Laurenz Albe <laurenz.albe@cybertec.at>
1 sibling, 0 replies; 18+ messages in thread
From: Aleksander Alekseev @ 2025-04-10 11:48 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org; +Cc: Laurenz Albe <laurenz.albe@cybertec.at>; Daniel Gustafsson <dgustafsson@postgresql.org>
Hi,
> 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.
I agree that the name is too generic for an exported symbol.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-10 12:46 Daniel Gustafsson <daniel@yesql.se>
parent: Laurenz Albe <laurenz.albe@cybertec.at>
1 sibling, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2025-04-10 12:46 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: pgsql-committers@lists.postgresql.org
> On 10 Apr 2025, at 13:42, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> On Tue, 2025-04-08 at 09:10 +0000, Daniel Gustafsson wrote:
>> +extern dsa_area *area;
>
> This commit causes problems for PostGIS, because the name "area" collides
> with a PostGIS object:
Thanks for the report, I've already posted a patch [0] and will push that
shortly.
--
Daniel Gustafsson
[0] https://postgr.es/m/68638ACC-3556-429E-93A0-189F73D0E274@yesql.se
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-10 12:51 Laurenz Albe <laurenz.albe@cybertec.at>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 0 replies; 18+ messages in thread
From: Laurenz Albe @ 2025-04-10 12:51 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-committers@lists.postgresql.org
On Thu, 2025-04-10 at 14:46 +0200, Daniel Gustafsson wrote:
> > On 10 Apr 2025, at 13:42, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > > +extern dsa_area *area;
> >
> > This commit causes problems for PostGIS, because the name "area" collides
> > with a PostGIS object:
>
> Thanks for the report, I've already posted a patch [0] and will push that
> shortly.
Great, thanks.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-10 13:31 Robert Haas <robertmhaas@gmail.com>
parent: Daniel Gustafsson <dgustafsson@postgresql.org>
1 sibling, 1 reply; 18+ messages in thread
From: Robert Haas @ 2025-04-10 13:31 UTC (permalink / raw)
To: Daniel Gustafsson <dgustafsson@postgresql.org>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On Tue, Apr 8, 2025 at 5:10 AM Daniel Gustafsson
<dgustafsson@postgresql.org> wrote:
> Add function to get memory context stats for processes
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. If there is
some existing discussion of this that I should read, please point me
in the right direction; I didn't see anything in a quick look through
the commit.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-10 20:05 Andres Freund <andres@anarazel.de>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Andres Freund @ 2025-04-10 20:05 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
Hi,
On 2025-04-10 09:31:00 -0400, Robert Haas wrote:
> On Tue, Apr 8, 2025 at 5:10 AM Daniel Gustafsson
> <dgustafsson@postgresql.org> wrote:
> > Add function to get memory context stats for processes
>
> 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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-14 14:03 Robert Haas <robertmhaas@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 18+ messages in thread
From: Robert Haas @ 2025-04-14 14:03 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On Thu, Apr 10, 2025 at 4:05 PM Andres Freund <andres@anarazel.de> wrote:
> 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?
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-14 14:13 Andres Freund <andres@anarazel.de>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Andres Freund @ 2025-04-14 14:13 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
Hi,
On 2025-04-14 10:03:28 -0400, Robert Haas wrote:
> On Thu, Apr 10, 2025 at 4:05 PM Andres Freund <andres@anarazel.de> wrote:
> > 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?
>
> 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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-17 14:42 Robert Haas <robertmhaas@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 2 replies; 18+ messages in thread
From: Robert Haas @ 2025-04-17 14:42 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On Tue, Apr 15, 2025 at 6:11 AM Andres Freund <andres@anarazel.de> wrote:
> 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?
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 do not believe we have
anywhere enough guarantees about reentrancy for that to be safe.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-22 08:11 Daniel Gustafsson <dgustafsson@postgresql.org>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2025-04-22 08:11 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
> On 17 Apr 2025, at 16:42, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Apr 15, 2025 at 6:11 AM Andres Freund <andres@anarazel.de> wrote:
>> 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?
>
> 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 do not believe we have
> anywhere enough guarantees about reentrancy for that to be safe.
Do you mean that an interrupt handler will make DSA allocations? I don't think
that would be something we'd want to allow regardless of this patch. Or did I
misunderstand the above?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-22 16:58 Andres Freund <andres@anarazel.de>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 18+ messages in thread
From: Andres Freund @ 2025-04-22 16:58 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
Hi,
On 2025-04-17 10:42:45 -0400, Robert Haas wrote:
> On Tue, Apr 15, 2025 at 6:11 AM Andres Freund <andres@anarazel.de> wrote:
> > 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?
>
> 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
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-23 16:12 Robert Haas <robertmhaas@gmail.com>
parent: Daniel Gustafsson <dgustafsson@postgresql.org>
0 siblings, 1 reply; 18+ messages in thread
From: Robert Haas @ 2025-04-23 16:12 UTC (permalink / raw)
To: Daniel Gustafsson <dgustafsson@postgresql.org>; +Cc: Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On Tue, Apr 22, 2025 at 4:12 AM Daniel Gustafsson
<dgustafsson@postgresql.org> wrote:
> Do you mean that an interrupt handler will make DSA allocations? I don't think
> that would be something we'd want to allow regardless of this patch. Or did I
> misunderstand the above?
My primary concern about the patch is that
ProcessGetMemoryContextInterrupt() can be called from any
CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including
dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm
shocked to hear that you and Andres think that's safe to do at any
current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but
Andres seems very confident that it's fine, so perhaps I should just
stop worrying and be happy that we have the feature.
I thought that the issues here would be similar to
https://commitfest.postgresql.org/patch/5473/ and
https://commitfest.postgresql.org/patch/5330/ where it seems we need
to go to fairly extraordinary lengths to try to make the operation
safe -- the proposal there is essentially to have a CFI handler run
around the executor state tree and replace every ExecProcNode pointer
with a pointer to some new wrapper function which in turn does the
actual query-plan printing. Even that requires some tricky footwork to
get right, but the core idea is that you can't imagine that
CHECK_FOR_INTERRUPTS() is a safe place to do arbitrary stuff, so you
have to use it to trigger the actual work at some later point where it
will be safe to do the stuff that you want to do. I suppose the
difference in this case is that memory-allocation is a lower-level
subsystem than query execution and therefore there are presumably
fewer places where it's not safe to assume that you can do
memory-allocation type operations. But I at least feel like DSA is a
pretty high-level system that I wouldn't want to count on being able
to access from a fairly-arbitrary point in the code, which is why I'm
quite astonished to hear Andres basically saying "don't worry, it's
all fine!". But my astonishment does not mean that he is wrong.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-23 18:14 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Tom Lane @ 2025-04-23 18:14 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
Robert Haas <robertmhaas@gmail.com> writes:
> My primary concern about the patch is that
> ProcessGetMemoryContextInterrupt() can be called from any
> CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including
> dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm
> shocked to hear that you and Andres think that's safe to do at any
> current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but
> Andres seems very confident that it's fine, so perhaps I should just
> stop worrying and be happy that we have the feature.
Just for the record, it sounds quite unsafe to me too. I could
credit it being all right to examine the process' MemoryContext data
structures, but calling dsa_create() from CFI seems really insane.
Way too many moving parts there.
regards, tom lane
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-26 21:07 Tomas Vondra <tomas@vondra.me>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 18+ messages in thread
From: Tomas Vondra @ 2025-04-26 21:07 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; +Cc: Daniel Gustafsson <dgustafsson@postgresql.org>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On 4/23/25 20:14, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> My primary concern about the patch is that
>> ProcessGetMemoryContextInterrupt() can be called from any
>> CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including
>> dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm
>> shocked to hear that you and Andres think that's safe to do at any
>> current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but
>> Andres seems very confident that it's fine, so perhaps I should just
>> stop worrying and be happy that we have the feature.
>
> Just for the record, it sounds quite unsafe to me too. I could
> credit it being all right to examine the process' MemoryContext data
> structures, but calling dsa_create() from CFI seems really insane.
> Way too many moving parts there.
>
Maybe I'm oblivious to some very obvious issues, but why is this so
different from everything else that is already called from
ProcessInterrupts()? Perhaps dsa_create() is more complex compared to
the other stuff (haven't checked), but why would it be unsafe?
The one risk I can think of is a risk of recursion - if any of the
functions called from ProcessGetMemoryContextInterrupt() does CFI, could
that be a problem if there's another pending signal. I see some other
handlers (e.g. ProcessParallelApplyMessages) handle this by explicitly
holding interrupts. Should ProcessGetMemoryContextInterrupt() do the
same thing?
In any case, if DSA happens to not be the right way to transfer this,
what should we use instead? The only thing I can think of is some sort
of pre-allocated chunk of shared memory.
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-30 20:17 Robert Haas <robertmhaas@gmail.com>
parent: Tomas Vondra <tomas@vondra.me>
0 siblings, 1 reply; 18+ messages in thread
From: Robert Haas @ 2025-04-30 20:17 UTC (permalink / raw)
To: Tomas Vondra <tomas@vondra.me>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Daniel Gustafsson <dgustafsson@postgresql.org>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On Sat, Apr 26, 2025 at 5:07 PM Tomas Vondra <tomas@vondra.me> wrote:
> > Just for the record, it sounds quite unsafe to me too. I could
> > credit it being all right to examine the process' MemoryContext data
> > structures, but calling dsa_create() from CFI seems really insane.
> > Way too many moving parts there.
>
> Maybe I'm oblivious to some very obvious issues, but why is this so
> different from everything else that is already called from
> ProcessInterrupts()? Perhaps dsa_create() is more complex compared to
> the other stuff (haven't checked), but why would it be unsafe?
>
> The one risk I can think of is a risk of recursion - if any of the
> functions called from ProcessGetMemoryContextInterrupt() does CFI, could
> that be a problem if there's another pending signal. I see some other
> handlers (e.g. ProcessParallelApplyMessages) handle this by explicitly
> holding interrupts. Should ProcessGetMemoryContextInterrupt() do the
> same thing?
>
> In any case, if DSA happens to not be the right way to transfer this,
> what should we use instead? The only thing I can think of is some sort
> of pre-allocated chunk of shared memory.
The big disadvantage of a pre-allocated chunk of shared memory is that
it would necessarily be fixed size, and a memory context dump can be
really big. Another alternative would be a temporary file. But none of
that settles the question of safety.
I think part of the reason why it's possible for us to have
disagreements about whether this is safe is that we don't have any
clear documentation of what you can assume to be true at a
CHECK_FOR_INTERRUPTS(). It's not possible for there to be an LWLock
held at that point, because we hold off interrupts when you acquire an
LWLock and don't re-enable them until all LWLocks have been released.
We can't be holding a spinlock, either, because we only insert
CHECK_FOR_INTERRUPTS() at the top of loops and code that holds a
spinlock is supposed to be straight-line code that never loops. But
what else can you assume? Can you assume, for example, that there's a
transaction? I doubt it. Can you assume that the transaction is
non-aborted? I doubt that even more. There's no obvious-to-me reason
why those things should be true.
And in fact if you try this on a backend waiting in an aborted
transaction, it breaks:
robert.haas=# select pg_backend_pid();
pg_backend_pid
----------------
19321
(1 row)
robert.haas=# begin;
BEGIN
robert.haas=*# select 1/0;
ERROR: division by zero
And then from another session, run this command, using the PID from above:
select * from pg_get_process_memory_contexts(19321, false, 1);
Then you get:
2025-04-30 15:14:33.878 EDT [19321] ERROR: ResourceOwnerEnlarge
called after release started
2025-04-30 15:14:33.878 EDT [19321] FATAL: terminating connection
because protocol synchronization was lost
I kind of doubt whether that's the only problem here, but it's *a* problem.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-30 20:29 Daniel Gustafsson <dgustafsson@postgresql.org>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Gustafsson @ 2025-04-30 20:29 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tomas Vondra <tomas@vondra.me>; Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
> On 30 Apr 2025, at 22:17, Robert Haas <robertmhaas@gmail.com> 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] 3eb40b3e-45c7-426a-b7f8-81f7d05a9b53@oss.nttdata.com
^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: pgsql: Add function to get memory context stats for processes
@ 2025-04-30 22:00 Robert Haas <robertmhaas@gmail.com>
parent: Daniel Gustafsson <dgustafsson@postgresql.org>
0 siblings, 0 replies; 18+ messages in thread
From: Robert Haas @ 2025-04-30 22:00 UTC (permalink / raw)
To: Daniel Gustafsson <dgustafsson@postgresql.org>; +Cc: Tomas Vondra <tomas@vondra.me>; Tom Lane <tgl@sss.pgh.pa.us>; Andres Freund <andres@anarazel.de>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Rahila Syed <rahilasyed90@gmail.com>
On Wed, Apr 30, 2025 at 4:29 PM Daniel Gustafsson
<dgustafsson@postgresql.org> wrote:
> 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.
Thanks for the pointer. I will reply to that thread.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2025-04-30 22:00 UTC | newest]
Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-08 09:10 pgsql: Add function to get memory context stats for processes Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-10 11:42 ` Laurenz Albe <laurenz.albe@cybertec.at>
2025-04-10 11:48 ` Aleksander Alekseev <aleksander@timescale.com>
2025-04-10 12:46 ` Daniel Gustafsson <daniel@yesql.se>
2025-04-10 12:51 ` Laurenz Albe <laurenz.albe@cybertec.at>
2025-04-10 13:31 ` Robert Haas <robertmhaas@gmail.com>
2025-04-10 20:05 ` Andres Freund <andres@anarazel.de>
2025-04-14 14:03 ` Robert Haas <robertmhaas@gmail.com>
2025-04-14 14:13 ` Andres Freund <andres@anarazel.de>
2025-04-17 14:42 ` Robert Haas <robertmhaas@gmail.com>
2025-04-22 08:11 ` Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-23 16:12 ` Robert Haas <robertmhaas@gmail.com>
2025-04-23 18:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-04-26 21:07 ` Tomas Vondra <tomas@vondra.me>
2025-04-30 20:17 ` Robert Haas <robertmhaas@gmail.com>
2025-04-30 20:29 ` Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 22:00 ` Robert Haas <robertmhaas@gmail.com>
2025-04-22 16:58 ` Andres Freund <andres@anarazel.de>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox