Prevent an error on attaching/creating a DSM/DSA from an interrupt handler.
Hi,
If a DSM is created or attached from an interrupt handler while a
transaction is being
rolled back, it may result in the following error.
"ResourceOwnerEnlarge called after release started"
This was found during the testing of Enhancing Memory Context Reporting
feature
by Fujii Masao [1]. /messages/by-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3@oss.nttdata.com </messages/by-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3@oss.nttdata.com>.
I propose a fix to fall back to creating a DSM or DSA with a NULL resource
owner if
CurrentResourceOwner is set to "releasing".
Since a DSM or DSA can already be created with a NULL resource owner—
meaning it persists for the lifetime of the backend or until explicitly
detached—
this approach should be acceptable.
Please find attached a patch which does that. Kindly let me know your views.
[1]: . /messages/by-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3@oss.nttdata.com </messages/by-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3@oss.nttdata.com>
/messages/by-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3@oss.nttdata.com
</messages/by-id/a1a7e2b7-8f33-4313-baff-42e92ec14fd3@oss.nttdata.com>
Attachments:
0001-Prevent-the-error-on-creating-a-dsm-segment-from-an-.patchapplication/octet-stream; name=0001-Prevent-the-error-on-creating-a-dsm-segment-from-an-.patchDownload
From 25282054756d9f2743cb6e4702b7de25828455ed Mon Sep 17 00:00:00 2001
From: Rahila Syed <rahilasyed.90@gmail.com>
Date: Tue, 14 Jan 2025 11:30:36 +0530
Subject: [PATCH] Prevent the error on creating a dsm segment from an interrupt
handler.
If DSA or DSM segment is attached to or created while
a transaction's resource owner is being released, it
will throw an error. Check whether the resource owner is
being released before adding the segment to that
resource owner.
---
src/backend/storage/ipc/dsm.c | 25 +++++++++++++++++--------
src/backend/utils/mmgr/dsa.c | 10 ++++++++--
src/backend/utils/resowner/resowner.c | 9 +++++++++
src/include/utils/resowner.h | 1 +
4 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f92a52a00e..a62ecdd003 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -934,9 +934,16 @@ void
dsm_unpin_mapping(dsm_segment *seg)
{
Assert(seg->resowner == NULL);
- ResourceOwnerEnlarge(CurrentResourceOwner);
- seg->resowner = CurrentResourceOwner;
- ResourceOwnerRememberDSM(seg->resowner, seg);
+
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ ResourceOwnerEnlarge(CurrentResourceOwner);
+ seg->resowner = CurrentResourceOwner;
+ ResourceOwnerRememberDSM(seg->resowner, seg);
+ }
+ else
+ /* Could not unpin */
+ seg->resowner = NULL;
}
/*
@@ -1202,9 +1209,6 @@ dsm_create_descriptor(void)
{
dsm_segment *seg;
- if (CurrentResourceOwner)
- ResourceOwnerEnlarge(CurrentResourceOwner);
-
seg = MemoryContextAlloc(TopMemoryContext, sizeof(dsm_segment));
dlist_push_head(&dsm_segment_list, &seg->node);
@@ -1214,9 +1218,14 @@ dsm_create_descriptor(void)
seg->mapped_address = NULL;
seg->mapped_size = 0;
- seg->resowner = CurrentResourceOwner;
- if (CurrentResourceOwner)
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ ResourceOwnerEnlarge(CurrentResourceOwner);
+ seg->resowner = CurrentResourceOwner;
ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+ }
+ else
+ seg->resowner = NULL;
slist_init(&seg->on_detach);
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 17d4f7a7a0..7b2c5a4015 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -1282,7 +1282,10 @@ create_internal(void *place, size_t size,
*/
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
area->freed_segment_counter = 0;
@@ -1338,7 +1341,10 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle)
/* Build the backend-local area object. */
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(&area->segment_maps[0], 0,
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index ac5ca4a765..60752e455d 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -1082,3 +1082,12 @@ ResourceOwnerForgetLock(ResourceOwner owner, LOCALLOCK *locallock)
elog(ERROR, "lock reference %p is not owned by resource owner %s",
locallock, owner->name);
}
+
+/*
+ * Returns true if resource owner is being released
+ */
+bool
+IsResourceOwnerReleasing(ResourceOwner owner)
+{
+ return(owner->releasing);
+}
diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h
index e8d452ca7e..dea97b9c0d 100644
--- a/src/include/utils/resowner.h
+++ b/src/include/utils/resowner.h
@@ -163,5 +163,6 @@ extern void ReleaseAuxProcessResources(bool isCommit);
struct LOCALLOCK;
extern void ResourceOwnerRememberLock(ResourceOwner owner, struct LOCALLOCK *locallock);
extern void ResourceOwnerForgetLock(ResourceOwner owner, struct LOCALLOCK *locallock);
+extern bool IsResourceOwnerReleasing(ResourceOwner owner);
#endif /* RESOWNER_H */
--
2.34.1
Hi,
Please find the attached updated and rebased patch.
I have added a test in the test_dsa module that uses a function
to create a dsa area. This function is called after
the resowner->releasing is set to true, using an injection point.
Thank you,
Rahila Syed
Attachments:
v2-0001-Prevent-the-error-on-creating-a-dsm-segment-from-an-.patchapplication/octet-stream; name=v2-0001-Prevent-the-error-on-creating-a-dsm-segment-from-an-.patchDownload
From 8cf1c9606fba2dacb69173727f8567e94298fd2f Mon Sep 17 00:00:00 2001
From: Rahila Syed <rahilasyed.90@gmail.com>
Date: Tue, 14 Jan 2025 11:30:36 +0530
Subject: [PATCH] Prevent the error on creating a dsm segment from an interrupt
handler.
If DSA or DSM segment is attached to or created while
a transaction's resource owner is being released, it
will throw an error. Check whether the resource owner is
being released before adding the segment to that
resource owner.
---
src/backend/storage/ipc/dsm.c | 29 +++++++---
src/backend/utils/mmgr/dsa.c | 10 +++-
src/backend/utils/resowner/resowner.c | 11 ++++
src/include/utils/resowner.h | 1 +
.../modules/test_dsa/expected/test_dsa.out | 10 ++++
src/test/modules/test_dsa/sql/test_dsa.sql | 5 ++
src/test/modules/test_dsa/test_dsa--1.0.sql | 4 ++
src/test/modules/test_dsa/test_dsa.c | 53 +++++++++++++++++++
8 files changed, 113 insertions(+), 10 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f92a52a00e..53d5d1dd78 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -934,9 +934,20 @@ void
dsm_unpin_mapping(dsm_segment *seg)
{
Assert(seg->resowner == NULL);
- ResourceOwnerEnlarge(CurrentResourceOwner);
- seg->resowner = CurrentResourceOwner;
- ResourceOwnerRememberDSM(seg->resowner, seg);
+
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ ResourceOwnerEnlarge(CurrentResourceOwner);
+ seg->resowner = CurrentResourceOwner;
+ ResourceOwnerRememberDSM(seg->resowner, seg);
+ }
+ else
+ {
+ /* Log failure of unpinning */
+ elog(DEBUG2, "unable to unpin the segment %u as CurrentResourceOwner is NULL or releasing",
+ seg);
+ seg->resowner = NULL;
+ }
}
/*
@@ -1202,9 +1213,6 @@ dsm_create_descriptor(void)
{
dsm_segment *seg;
- if (CurrentResourceOwner)
- ResourceOwnerEnlarge(CurrentResourceOwner);
-
seg = MemoryContextAlloc(TopMemoryContext, sizeof(dsm_segment));
dlist_push_head(&dsm_segment_list, &seg->node);
@@ -1214,9 +1222,14 @@ dsm_create_descriptor(void)
seg->mapped_address = NULL;
seg->mapped_size = 0;
- seg->resowner = CurrentResourceOwner;
- if (CurrentResourceOwner)
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ ResourceOwnerEnlarge(CurrentResourceOwner);
+ seg->resowner = CurrentResourceOwner;
ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+ }
+ else
+ seg->resowner = NULL;
slist_init(&seg->on_detach);
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 17d4f7a7a0..7b2c5a4015 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -1282,7 +1282,10 @@ create_internal(void *place, size_t size,
*/
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
area->freed_segment_counter = 0;
@@ -1338,7 +1341,10 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle)
/* Build the backend-local area object. */
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(&area->segment_maps[0], 0,
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index d39f3e1b65..fe506a7451 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -52,6 +52,7 @@
#include "storage/ipc.h"
#include "storage/predicate.h"
#include "storage/proc.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/resowner.h"
@@ -702,6 +703,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
Assert(phase == RESOURCE_RELEASE_BEFORE_LOCKS);
Assert(!owner->sorted);
owner->releasing = true;
+ INJECTION_POINT("dsa_create_on_res_release");
}
else
{
@@ -1111,3 +1113,12 @@ ResourceOwnerForgetAioHandle(ResourceOwner owner, struct dlist_node *ioh_node)
{
dlist_delete_from(&owner->aio_handles, ioh_node);
}
+
+/*
+ * Returns true if resource owner is being released
+ */
+bool
+IsResourceOwnerReleasing(ResourceOwner owner)
+{
+ return (owner->releasing);
+}
diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h
index aede4bfc82..7b4c3765a1 100644
--- a/src/include/utils/resowner.h
+++ b/src/include/utils/resowner.h
@@ -163,6 +163,7 @@ extern void ReleaseAuxProcessResources(bool isCommit);
struct LOCALLOCK;
extern void ResourceOwnerRememberLock(ResourceOwner owner, struct LOCALLOCK *locallock);
extern void ResourceOwnerForgetLock(ResourceOwner owner, struct LOCALLOCK *locallock);
+extern bool IsResourceOwnerReleasing(ResourceOwner owner);
/* special support for AIO */
struct dlist_node;
diff --git a/src/test/modules/test_dsa/expected/test_dsa.out b/src/test/modules/test_dsa/expected/test_dsa.out
index 266010e77f..9c67fb0c0e 100644
--- a/src/test/modules/test_dsa/expected/test_dsa.out
+++ b/src/test/modules/test_dsa/expected/test_dsa.out
@@ -11,3 +11,13 @@ SELECT test_dsa_resowners();
(1 row)
+SELECT test_dsa_injection('dsa_create_on_res_release');
+ test_dsa_injection
+--------------------
+
+(1 row)
+
+begin;
+select 1/0;
+ERROR: division by zero
+commit;
diff --git a/src/test/modules/test_dsa/sql/test_dsa.sql b/src/test/modules/test_dsa/sql/test_dsa.sql
index c3d8db9437..a06ae84594 100644
--- a/src/test/modules/test_dsa/sql/test_dsa.sql
+++ b/src/test/modules/test_dsa/sql/test_dsa.sql
@@ -2,3 +2,8 @@ CREATE EXTENSION test_dsa;
SELECT test_dsa_basic();
SELECT test_dsa_resowners();
+SELECT test_dsa_injection('dsa_create_on_res_release');
+
+begin;
+select 1/0;
+commit;
diff --git a/src/test/modules/test_dsa/test_dsa--1.0.sql b/src/test/modules/test_dsa/test_dsa--1.0.sql
index 2904cb2352..4f0e655667 100644
--- a/src/test/modules/test_dsa/test_dsa--1.0.sql
+++ b/src/test/modules/test_dsa/test_dsa--1.0.sql
@@ -10,3 +10,7 @@ CREATE FUNCTION test_dsa_basic()
CREATE FUNCTION test_dsa_resowners()
RETURNS pg_catalog.void
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION test_dsa_injection(IN point_name TEXT)
+ RETURNS pg_catalog.void
+ AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_dsa/test_dsa.c b/src/test/modules/test_dsa/test_dsa.c
index cd24d0f487..2505139241 100644
--- a/src/test/modules/test_dsa/test_dsa.c
+++ b/src/test/modules/test_dsa/test_dsa.c
@@ -14,10 +14,14 @@
#include "fmgr.h"
#include "storage/lwlock.h"
+#include "utils/builtins.h"
#include "utils/dsa.h"
+#include "utils/injection_point.h"
#include "utils/resowner.h"
PG_MODULE_MAGIC;
+extern PGDLLEXPORT void test_dsa_inj(const char *name,
+ const void *private_data);
/* Test basic DSA functionality */
PG_FUNCTION_INFO_V1(test_dsa_basic);
@@ -111,3 +115,52 @@ test_dsa_resowners(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+
+PG_FUNCTION_INFO_V1(test_dsa_injection);
+Datum
+test_dsa_injection(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *function = "test_dsa_inj";
+
+ InjectionPointAttach(name, "test_dsa", function, NULL,
+ 0);
+
+ PG_RETURN_VOID();
+}
+
+void
+test_dsa_inj(const char *name, const void *private_data)
+{
+ dsa_area *a;
+ dsa_pointer p[100];
+ int tranche_id;
+
+ /* XXX: this tranche is leaked */
+ tranche_id = LWLockNewTrancheId();
+ LWLockRegisterTranche(tranche_id, "test_dsa");
+
+ a = dsa_create(tranche_id);
+ for (int i = 0; i < 100; i++)
+ {
+ p[i] = dsa_allocate(a, 1000);
+ snprintf(dsa_get_address(a, p[i]), 1000, "foobar%d", i);
+ }
+
+ for (int i = 0; i < 100; i++)
+ {
+ char buf[100];
+
+ snprintf(buf, 100, "foobar%d", i);
+ if (strcmp(dsa_get_address(a, p[i]), buf) != 0)
+ elog(ERROR, "no match");
+ }
+
+ for (int i = 0; i < 100; i++)
+ {
+ dsa_free(a, p[i]);
+ }
+
+ dsa_detach(a);
+ return;
+}
--
2.34.1
On 24 Mar 2025, at 20:31, Rahila Syed <rahilasyed90@gmail.com> wrote:
Please find the attached updated and rebased patch.
Thanks for this rebase, as was mentioned in the other thread I plan to get this
committed fairly soon. A few comments on the code, all performed in the
attached v3.
+ else
+ {
+ /* Log failure of unpinning */
+ elog(DEBUG2, "unable to unpin the segment %u as CurrentResourceOwner is NULL or releasing",
+ seg);
+ seg->resowner = NULL;
+ }
I removed the elog() calls since I can't see it adding enough value, and the
assignment to NULL can be removed as well since we've already asserted that
seg->resowner is NULL.
+ INJECTION_POINT("dsa_create_on_res_release");
This feels like a name which limits its use to this one test, whereas it is a
general purpose injection point. Renamed, and also moved to using dashes
rather than underscore as the former is project style.
+void
+test_dsa_inj(const char *name, const void *private_data)
Rather than duplicating the code I created an internal function for this test
which can be called from the existing basic test as well as this new test.
I also did a little bit of renaming to make it more readable.
As it can only really be tested with an injection point I plan on only
backpatching to 17 initially. Searching the archives I didn't find any mention
of this bug ever being hit so it seems safe to let it prove itself in testable
versions before going further back with it.
--
Daniel Gustafsson
Attachments:
v3-0001-Don-t-try-to-enlarge-resourceowner-when-releasing.patchapplication/octet-stream; name=v3-0001-Don-t-try-to-enlarge-resourceowner-when-releasing.patch; x-unix-mode=0644Download
From c70887675a7c981614d16d085e7cf2131e406455 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 11 Apr 2025 11:50:47 +0200
Subject: [PATCH v3] Don't try to enlarge resourceowner when releasing
If DSA or DSM segment is attached to, or created, while a
transaction's resource owner is being released, it throws
an error when trying to enlarge the resource owner.
Session 1 (PID=1000):
=# begin;
=# select 1/0;
ERROR: division by zero
Session 2:
=# select * from pg_get_process_memory_contexts(1000,false,1);
Session 1 terminates with:
ERROR: ResourceOwnerEnlarge called after release started
FATAL: terminating connection because protocol synchronization was lost
Fix by checking whether the resource owner is being released
before adding the segment to it. In order to test this an
injection point is added. While the bug exists further back,
backpatching is currently limited to v17 where the injection
point framework was introduced as it cannot be easily tested
without it.
Author: Rahila Syed <rahilasyed90@gmail.com>
Reported-by: Fujii Masao <masao.fujii@oss.nttdata.com>
Discussion: https://postgr.es/m/a1a7e2b7-8f33-4313-baff-42e92ec14fd3%40oss.nttdata.com
Discussion: https://postgr.es/m/CAH2L28shr0j3JE5V3CXDFmDH-agTSnh2V8pR23X0UhRMbDQD9Q@mail.gmail.com
Backpatch-through: 17
---
src/backend/storage/ipc/dsm.c | 16 ++++---
src/backend/utils/mmgr/dsa.c | 10 ++++-
src/backend/utils/resowner/resowner.c | 11 +++++
src/include/utils/resowner.h | 1 +
.../modules/test_dsa/expected/test_dsa.out | 10 +++++
src/test/modules/test_dsa/sql/test_dsa.sql | 5 +++
src/test/modules/test_dsa/test_dsa--1.0.sql | 4 ++
src/test/modules/test_dsa/test_dsa.c | 44 +++++++++++++++++--
8 files changed, 90 insertions(+), 11 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f92a52a00e6..1227b0d0b45 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -934,6 +934,10 @@ void
dsm_unpin_mapping(dsm_segment *seg)
{
Assert(seg->resowner == NULL);
+
+ if (CurrentResourceOwner && IsResourceOwnerReleasing(CurrentResourceOwner))
+ return;
+
ResourceOwnerEnlarge(CurrentResourceOwner);
seg->resowner = CurrentResourceOwner;
ResourceOwnerRememberDSM(seg->resowner, seg);
@@ -1202,9 +1206,6 @@ dsm_create_descriptor(void)
{
dsm_segment *seg;
- if (CurrentResourceOwner)
- ResourceOwnerEnlarge(CurrentResourceOwner);
-
seg = MemoryContextAlloc(TopMemoryContext, sizeof(dsm_segment));
dlist_push_head(&dsm_segment_list, &seg->node);
@@ -1214,9 +1215,14 @@ dsm_create_descriptor(void)
seg->mapped_address = NULL;
seg->mapped_size = 0;
- seg->resowner = CurrentResourceOwner;
- if (CurrentResourceOwner)
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ ResourceOwnerEnlarge(CurrentResourceOwner);
+ seg->resowner = CurrentResourceOwner;
ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+ }
+ else
+ seg->resowner = NULL;
slist_init(&seg->on_detach);
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 17d4f7a7a06..7b2c5a40157 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -1282,7 +1282,10 @@ create_internal(void *place, size_t size,
*/
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
area->freed_segment_counter = 0;
@@ -1338,7 +1341,10 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle)
/* Build the backend-local area object. */
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(&area->segment_maps[0], 0,
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index d39f3e1b655..3ad4040523d 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -52,6 +52,7 @@
#include "storage/ipc.h"
#include "storage/predicate.h"
#include "storage/proc.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/resowner.h"
@@ -702,6 +703,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
Assert(phase == RESOURCE_RELEASE_BEFORE_LOCKS);
Assert(!owner->sorted);
owner->releasing = true;
+ INJECTION_POINT("resowner-release");
}
else
{
@@ -1111,3 +1113,12 @@ ResourceOwnerForgetAioHandle(ResourceOwner owner, struct dlist_node *ioh_node)
{
dlist_delete_from(&owner->aio_handles, ioh_node);
}
+
+/*
+ * Returns true if resource owner is being released
+ */
+bool
+IsResourceOwnerReleasing(ResourceOwner owner)
+{
+ return (owner->releasing);
+}
diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h
index aede4bfc820..7b4c3765a1f 100644
--- a/src/include/utils/resowner.h
+++ b/src/include/utils/resowner.h
@@ -163,6 +163,7 @@ extern void ReleaseAuxProcessResources(bool isCommit);
struct LOCALLOCK;
extern void ResourceOwnerRememberLock(ResourceOwner owner, struct LOCALLOCK *locallock);
extern void ResourceOwnerForgetLock(ResourceOwner owner, struct LOCALLOCK *locallock);
+extern bool IsResourceOwnerReleasing(ResourceOwner owner);
/* special support for AIO */
struct dlist_node;
diff --git a/src/test/modules/test_dsa/expected/test_dsa.out b/src/test/modules/test_dsa/expected/test_dsa.out
index 266010e77fe..e6bd8184143 100644
--- a/src/test/modules/test_dsa/expected/test_dsa.out
+++ b/src/test/modules/test_dsa/expected/test_dsa.out
@@ -11,3 +11,13 @@ SELECT test_dsa_resowners();
(1 row)
+SELECT inject_dsa_in_resowner_release();
+ inject_dsa_in_resowner_release
+--------------------------------
+
+(1 row)
+
+begin;
+select 1/0;
+ERROR: division by zero
+commit;
diff --git a/src/test/modules/test_dsa/sql/test_dsa.sql b/src/test/modules/test_dsa/sql/test_dsa.sql
index c3d8db94372..4d0c90bd5ec 100644
--- a/src/test/modules/test_dsa/sql/test_dsa.sql
+++ b/src/test/modules/test_dsa/sql/test_dsa.sql
@@ -2,3 +2,8 @@ CREATE EXTENSION test_dsa;
SELECT test_dsa_basic();
SELECT test_dsa_resowners();
+SELECT inject_dsa_in_resowner_release();
+
+begin;
+select 1/0;
+commit;
diff --git a/src/test/modules/test_dsa/test_dsa--1.0.sql b/src/test/modules/test_dsa/test_dsa--1.0.sql
index 2904cb23525..2629b469197 100644
--- a/src/test/modules/test_dsa/test_dsa--1.0.sql
+++ b/src/test/modules/test_dsa/test_dsa--1.0.sql
@@ -10,3 +10,7 @@ CREATE FUNCTION test_dsa_basic()
CREATE FUNCTION test_dsa_resowners()
RETURNS pg_catalog.void
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION inject_dsa_in_resowner_release()
+ RETURNS pg_catalog.void
+ AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_dsa/test_dsa.c b/src/test/modules/test_dsa/test_dsa.c
index cd24d0f4873..c9931c63d8b 100644
--- a/src/test/modules/test_dsa/test_dsa.c
+++ b/src/test/modules/test_dsa/test_dsa.c
@@ -14,15 +14,19 @@
#include "fmgr.h"
#include "storage/lwlock.h"
+#include "utils/builtins.h"
#include "utils/dsa.h"
+#include "utils/injection_point.h"
#include "utils/resowner.h"
PG_MODULE_MAGIC;
-/* Test basic DSA functionality */
-PG_FUNCTION_INFO_V1(test_dsa_basic);
-Datum
-test_dsa_basic(PG_FUNCTION_ARGS)
+static void test_dsa_common(void);
+
+extern PGDLLEXPORT void inj_create_dsa(const char *name, const void *private_data);
+
+static void
+test_dsa_common(void)
{
int tranche_id;
dsa_area *a;
@@ -54,7 +58,14 @@ test_dsa_basic(PG_FUNCTION_ARGS)
}
dsa_detach(a);
+}
+/* Test basic DSA functionality */
+PG_FUNCTION_INFO_V1(test_dsa_basic);
+Datum
+test_dsa_basic(PG_FUNCTION_ARGS)
+{
+ test_dsa_common();
PG_RETURN_VOID();
}
@@ -111,3 +122,28 @@ test_dsa_resowners(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+
+/*
+ * Test to create a DSA in a resource owner which is currently releasing.
+ */
+PG_FUNCTION_INFO_V1(inject_dsa_in_resowner_release);
+Datum
+inject_dsa_in_resowner_release(PG_FUNCTION_ARGS)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointAttach("resowner-release",
+ "test_dsa",
+ "inj_create_dsa",
+ NULL,
+ 0);
+#endif
+ PG_RETURN_VOID();
+}
+void
+inj_create_dsa(const char *name, const void *private_data)
+{
+ (void) name;
+ (void) private_data;
+
+ test_dsa_common();
+}
--
2.39.3 (Apple Git-146)
Hi Daniel,
Thank you for your review and code improvements.
Please find below some observations.
1. dsm_unpin_mapping(dsm_segment *seg)
+ if (CurrentResourceOwner &&
IsResourceOwnerReleasing(CurrentResourceOwner))
+ return;
Given that the function can return without setting resowner to a
CurrentResourceOwner which is not NULL, shall we change the function
signature to return true when "unpin" is successful and false when not?
This behavior existed earlier too, but adding the check has made it
explicit.
Although this function is not currently in use, it would be beneficial to
make
the API more verbose.
2. If value of IsResourceOwnerReleasing changes between
dsm_create_descriptor
and attach_internal, the dsm segment and dsa area will end up with
different resource owners.
Earlier the chances of CurrentResourceOwner changing between the two
functions were zero.
May be something can be done to keep resowner assignments under both these
functions
in sync.
Thank you,
Rahila Syed
On 11 Apr 2025, at 21:08, Rahila Syed <rahilasyed90@gmail.com> wrote:
Hi Daniel,
Thank you for your review and code improvements.
Please find below some observations.
1. dsm_unpin_mapping(dsm_segment *seg) + if (CurrentResourceOwner && IsResourceOwnerReleasing(CurrentResourceOwner)) + return;Given that the function can return without setting resowner to a
CurrentResourceOwner which is not NULL, shall we change the function
signature to return true when "unpin" is successful and false when not?
Maybe, but at this point in the cycle we cannot change the prototype like that.
Food for thought for v19 though.
2. If value of IsResourceOwnerReleasing changes between dsm_create_descriptor
and attach_internal, the dsm segment and dsa area will end up with different resource owners.
Earlier the chances of CurrentResourceOwner changing between the two functions were zero.
May be something can be done to keep resowner assignments under both these functions
in sync.
Hmm, that's a good question, not sure. Do you have a repro for this handy?
Attached is a current v4 with a few small tweaks.
--
Daniel Gustafsson
Attachments:
v4-0001-Don-t-try-to-enlarge-resourceowner-when-releasing.patchapplication/octet-stream; name=v4-0001-Don-t-try-to-enlarge-resourceowner-when-releasing.patch; x-unix-mode=0644Download
From 8cee8430327ab1922d9b3b82134e3eae7b1129b2 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 11 Apr 2025 11:50:47 +0200
Subject: [PATCH v4] Don't try to enlarge resourceowner when releasing
If DSA or DSM segment is attached to, or created, while a
transaction's resource owner is being released, it throws
an error when trying to enlarge the resource owner.
Session 1 (PID=1000):
=# begin;
=# select 1/0;
ERROR: division by zero
Session 2:
=# select * from pg_get_process_memory_contexts(1000,false,1);
Session 1 terminates with:
ERROR: ResourceOwnerEnlarge called after release started
FATAL: terminating connection because protocol synchronization was lost
Fix by checking whether the resource owner is being released
before adding the segment to it. In order to test this an
injection point is added. While the bug exists further back,
backpatching is currently limited to v17 where the injection
point framework was introduced as it cannot be easily tested
without it.
Author: Rahila Syed <rahilasyed90@gmail.com>
Reported-by: Fujii Masao <masao.fujii@oss.nttdata.com>
Discussion: https://postgr.es/m/a1a7e2b7-8f33-4313-baff-42e92ec14fd3%40oss.nttdata.com
Discussion: https://postgr.es/m/CAH2L28shr0j3JE5V3CXDFmDH-agTSnh2V8pR23X0UhRMbDQD9Q@mail.gmail.com
Backpatch-through: 17
---
src/backend/storage/ipc/dsm.c | 13 ++++--
src/backend/utils/mmgr/dsa.c | 10 ++++-
src/backend/utils/resowner/resowner.c | 8 ++++
src/include/utils/resowner.h | 1 +
.../modules/test_dsa/expected/test_dsa.out | 10 +++++
src/test/modules/test_dsa/sql/test_dsa.sql | 5 +++
src/test/modules/test_dsa/test_dsa--1.0.sql | 4 ++
src/test/modules/test_dsa/test_dsa.c | 44 +++++++++++++++++--
8 files changed, 86 insertions(+), 9 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f92a52a00e6..479983882bb 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -934,6 +934,10 @@ void
dsm_unpin_mapping(dsm_segment *seg)
{
Assert(seg->resowner == NULL);
+
+ if (CurrentResourceOwner && IsResourceOwnerReleasing(CurrentResourceOwner))
+ return;
+
ResourceOwnerEnlarge(CurrentResourceOwner);
seg->resowner = CurrentResourceOwner;
ResourceOwnerRememberDSM(seg->resowner, seg);
@@ -1202,7 +1206,7 @@ dsm_create_descriptor(void)
{
dsm_segment *seg;
- if (CurrentResourceOwner)
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
ResourceOwnerEnlarge(CurrentResourceOwner);
seg = MemoryContextAlloc(TopMemoryContext, sizeof(dsm_segment));
@@ -1213,10 +1217,13 @@ dsm_create_descriptor(void)
seg->impl_private = NULL;
seg->mapped_address = NULL;
seg->mapped_size = 0;
+ seg->resowner = NULL;
- seg->resowner = CurrentResourceOwner;
- if (CurrentResourceOwner)
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+ seg->resowner = CurrentResourceOwner;
+ }
slist_init(&seg->on_detach);
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 17d4f7a7a06..7b2c5a40157 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -1282,7 +1282,10 @@ create_internal(void *place, size_t size,
*/
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
area->freed_segment_counter = 0;
@@ -1338,7 +1341,10 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle)
/* Build the backend-local area object. */
area = palloc(sizeof(dsa_area));
area->control = control;
- area->resowner = CurrentResourceOwner;
+ if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner))
+ area->resowner = CurrentResourceOwner;
+ else
+ area->resowner = NULL;
memset(&area->segment_maps[0], 0,
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index d39f3e1b655..aec0ade61ec 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -52,6 +52,7 @@
#include "storage/ipc.h"
#include "storage/predicate.h"
#include "storage/proc.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/resowner.h"
@@ -702,6 +703,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
Assert(phase == RESOURCE_RELEASE_BEFORE_LOCKS);
Assert(!owner->sorted);
owner->releasing = true;
+ INJECTION_POINT("resowner-release");
}
else
{
@@ -1111,3 +1113,9 @@ ResourceOwnerForgetAioHandle(ResourceOwner owner, struct dlist_node *ioh_node)
{
dlist_delete_from(&owner->aio_handles, ioh_node);
}
+
+bool
+IsResourceOwnerReleasing(ResourceOwner owner)
+{
+ return owner->releasing;
+}
diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h
index aede4bfc820..7b4c3765a1f 100644
--- a/src/include/utils/resowner.h
+++ b/src/include/utils/resowner.h
@@ -163,6 +163,7 @@ extern void ReleaseAuxProcessResources(bool isCommit);
struct LOCALLOCK;
extern void ResourceOwnerRememberLock(ResourceOwner owner, struct LOCALLOCK *locallock);
extern void ResourceOwnerForgetLock(ResourceOwner owner, struct LOCALLOCK *locallock);
+extern bool IsResourceOwnerReleasing(ResourceOwner owner);
/* special support for AIO */
struct dlist_node;
diff --git a/src/test/modules/test_dsa/expected/test_dsa.out b/src/test/modules/test_dsa/expected/test_dsa.out
index 266010e77fe..e6bd8184143 100644
--- a/src/test/modules/test_dsa/expected/test_dsa.out
+++ b/src/test/modules/test_dsa/expected/test_dsa.out
@@ -11,3 +11,13 @@ SELECT test_dsa_resowners();
(1 row)
+SELECT inject_dsa_in_resowner_release();
+ inject_dsa_in_resowner_release
+--------------------------------
+
+(1 row)
+
+begin;
+select 1/0;
+ERROR: division by zero
+commit;
diff --git a/src/test/modules/test_dsa/sql/test_dsa.sql b/src/test/modules/test_dsa/sql/test_dsa.sql
index c3d8db94372..4d0c90bd5ec 100644
--- a/src/test/modules/test_dsa/sql/test_dsa.sql
+++ b/src/test/modules/test_dsa/sql/test_dsa.sql
@@ -2,3 +2,8 @@ CREATE EXTENSION test_dsa;
SELECT test_dsa_basic();
SELECT test_dsa_resowners();
+SELECT inject_dsa_in_resowner_release();
+
+begin;
+select 1/0;
+commit;
diff --git a/src/test/modules/test_dsa/test_dsa--1.0.sql b/src/test/modules/test_dsa/test_dsa--1.0.sql
index 2904cb23525..2629b469197 100644
--- a/src/test/modules/test_dsa/test_dsa--1.0.sql
+++ b/src/test/modules/test_dsa/test_dsa--1.0.sql
@@ -10,3 +10,7 @@ CREATE FUNCTION test_dsa_basic()
CREATE FUNCTION test_dsa_resowners()
RETURNS pg_catalog.void
AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION inject_dsa_in_resowner_release()
+ RETURNS pg_catalog.void
+ AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_dsa/test_dsa.c b/src/test/modules/test_dsa/test_dsa.c
index cd24d0f4873..c9931c63d8b 100644
--- a/src/test/modules/test_dsa/test_dsa.c
+++ b/src/test/modules/test_dsa/test_dsa.c
@@ -14,15 +14,19 @@
#include "fmgr.h"
#include "storage/lwlock.h"
+#include "utils/builtins.h"
#include "utils/dsa.h"
+#include "utils/injection_point.h"
#include "utils/resowner.h"
PG_MODULE_MAGIC;
-/* Test basic DSA functionality */
-PG_FUNCTION_INFO_V1(test_dsa_basic);
-Datum
-test_dsa_basic(PG_FUNCTION_ARGS)
+static void test_dsa_common(void);
+
+extern PGDLLEXPORT void inj_create_dsa(const char *name, const void *private_data);
+
+static void
+test_dsa_common(void)
{
int tranche_id;
dsa_area *a;
@@ -54,7 +58,14 @@ test_dsa_basic(PG_FUNCTION_ARGS)
}
dsa_detach(a);
+}
+/* Test basic DSA functionality */
+PG_FUNCTION_INFO_V1(test_dsa_basic);
+Datum
+test_dsa_basic(PG_FUNCTION_ARGS)
+{
+ test_dsa_common();
PG_RETURN_VOID();
}
@@ -111,3 +122,28 @@ test_dsa_resowners(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+
+/*
+ * Test to create a DSA in a resource owner which is currently releasing.
+ */
+PG_FUNCTION_INFO_V1(inject_dsa_in_resowner_release);
+Datum
+inject_dsa_in_resowner_release(PG_FUNCTION_ARGS)
+{
+#ifdef USE_INJECTION_POINTS
+ InjectionPointAttach("resowner-release",
+ "test_dsa",
+ "inj_create_dsa",
+ NULL,
+ 0);
+#endif
+ PG_RETURN_VOID();
+}
+void
+inj_create_dsa(const char *name, const void *private_data)
+{
+ (void) name;
+ (void) private_data;
+
+ test_dsa_common();
+}
--
2.39.3 (Apple Git-146)
On Wed, Apr 30, 2025 at 5:24 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Attached is a current v4 with a few small tweaks.
Sorry to turn up late here, but I strongly disagree with the notion
that this is a bug in the DSM or DSA code. It seems to me that it is
the caller's responsibility to provide a valid resource owner, not the
job of the called code to ignore the resource owner when it's
unusable. I suspect that there are many other parts of the system that
rely on the ResourceOwner machinery which likewise assume that the
ResourceOwner that they are passed is valid.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, Apr 30, 2025 at 06:03:49PM -0400, Robert Haas wrote:
Sorry to turn up late here, but I strongly disagree with the notion
that this is a bug in the DSM or DSA code. It seems to me that it is
the caller's responsibility to provide a valid resource owner, not the
job of the called code to ignore the resource owner when it's
unusable. I suspect that there are many other parts of the system that
rely on the ResourceOwner machinery which likewise assume that the
ResourceOwner that they are passed is valid.
Yeah, dshash would be one, I think. It feels to me that if you want
to enforce this kind of policy to be checked, this is something that
should be done in the shape of one or more assertion based the state
of the resource owner expected in these low-level paths rather than
tweaking the DSA and DSM code to do what you are expecting here, and
only enforce such new policies on HEAD to avoid disruption with
existing systems.
I'm actually rather scared of the patch, isn't there a risk of
breaking existing patterns that worked out of the box by forcing the
resowner to not be set? My spidey sense tingles when I see such
patterns, because this is enforcing assumptions directly hidden to the
callers.
--
Michael
On 1 May 2025, at 00:04, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Apr 30, 2025 at 5:24 PM Daniel Gustafsson <daniel@yesql.se> wrote:
Attached is a current v4 with a few small tweaks.
Sorry to turn up late here, but I strongly disagree with the notion
that this is a bug in the DSM or DSA code. It seems to me that it is
the caller's responsibility to provide a valid resource owner, not the
job of the called code to ignore the resource owner when it's
unusable. I suspect that there are many other parts of the system that
rely on the ResourceOwner machinery which likewise assume that the
ResourceOwner that they are passed is valid.
Thanks for review, I’m currently travelling over the extended weekend but will rework the approach shortly when back at the office.
./daniel
On Wed, Apr 30, 2025 at 6:56 PM Michael Paquier <michael@paquier.xyz> wrote:
I'm actually rather scared of the patch, isn't there a risk of
breaking existing patterns that worked out of the box by forcing the
resowner to not be set? My spidey sense tingles when I see such
patterns, because this is enforcing assumptions directly hidden to the
callers.
I'm not worried about this particular problem. We use resowner == NULL
in various places to mean "for the lifetime of the backend," which
makes a lot of sense if you think about it: the job of the resowner is
to release resources when the resowner is cleaned up; if you don't
ever want to release the resources, then you don't need a resowner.
What I'm concerned about is that I think that (as I said on the other
thread) is that ProcessGetMemoryContextInterrupt is not really at all
safe to execute at an arbitrary CHECK_FOR_INTERRUPTS(). In other
places where we use resource owners, we don't run into the problem
that happened here because we are careful about the timing of our
resource owner operations. You can see that caution in, for example,
AbortTransaction() and AbortSubTransaction(), where we make sure to
close down higher-level subsystems first, so that when we get to
lower-level cleanup, we know that no more resources are going to be
allocated afterward. But here, we are executing complex operations
that require a "clean" state at pretty much any point in the code,
where we don't actually know that things are sane enough for this code
to execute successfully. This particular error is just a symptom of
that more general problem.
In my mind, the possible fixes here are (1) revert that patch, (2)
redesign things so that the problematic code can only get called when
we know that the backend state is sane, or (3) redesign the code so
that it has fewer requirements for correct operation. Regarding (3),
this particular problem could maybe be solved by not relying on the
resowner machinery at all: we always want the DSA to be pinned, so if
we could create it pre-pinned rather than pinning it as an
after-the-fact step, we wouldn't need the resowner at all. Except that
I don't really think that would be entirely safe, because that seems
to open up the possibility that we'd just leak the DSA area entirely:
say you enter dsa_create with no resowner and then fail someplace
inside that function after you've allocated resources. There's nothing
to clean up those resources, but since you never returned the DSA
handle, it's not stored anywhere, so those resources have just been
leaked. I think this is actually why the interface works the way it
does. Maybe there's some way to fix it by creating a temporary
resource owner that is used by just this code ... but in general I'm
skeptical that we can really set up something that is OK to do in an
aborted transaction, because our ability to handle any further errors
at that point is extremely limited, and this code is definitely
complex enough that it could error out.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi,
On Thu, May 1, 2025 at 4:26 AM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Apr 30, 2025 at 06:03:49PM -0400, Robert Haas wrote:
Sorry to turn up late here, but I strongly disagree with the notion
that this is a bug in the DSM or DSA code. It seems to me that it is
the caller's responsibility to provide a valid resource owner, not the
job of the called code to ignore the resource owner when it's
unusable. I suspect that there are many other parts of the system that
rely on the ResourceOwner machinery which likewise assume that the
ResourceOwner that they are passed is valid.Yeah, dshash would be one, I think. It feels to me that if you want
to enforce this kind of policy to be checked, this is something that
should be done in the shape of one or more assertion based the state
of the resource owner expected in these low-level paths rather than
tweaking the DSA and DSM code to do what you are expecting here, and
only enforce such new policies on HEAD to avoid disruption with
existing systems.
I believe it would be particularly useful to add an assertion in
dsm_unpin_mapping().
This function relies on CurrentResourceOwner being non-NULL and not
releasing to
successfully unpin a mapping.
I'm actually rather scared of the patch, isn't there a risk of
breaking existing patterns that worked out of the box by forcing the
resowner to not be set?
In this context, resowner not being set means that the dsm segment
would remain attached until the session ends or until it is explicitly
detached. IIUC, the key difference is that a segment will stay mapped
for longer than expected in cases where the mapping was created
when a transaction was aborting.
Thank you for the review comments, it makes sense to include this
check in the callers of the DSA API.
Wrapping the dsa_create/dsa_attach call in the following snippet helps
resolve the issue in case of ProcessGetMemoryContextInterrupt.
+ ResourceOwner current_owner;
+ bool res_releasing = false;
+
+ if (CurrentResourceOwner &&
IsResourceOwnerReleasing(CurrentResourceOwner))
+ {
+ current_owner = CurrentResourceOwner;
+ CurrentResourceOwner = NULL;
+ res_releasing = true;
+ }
+
MemoryStatsDsaArea =
dsa_create(memCxtArea->lw_lock.tranche);
+ if (res_releasing)
+ CurrentResourceOwner = current_owner;
Kindly let me know your views.
Thank you,
Rahila Syed
Hello, everyone.
Not sure if it helps but encountered the same problem in relation to
injection points:
/messages/by-id/CANtu0oiTgFW47QgpTwrMOVm3Bq4N0Y5bjvTy5sP0gYWLQuVgjw@mail.gmail.com
On 1 May 2025, at 14:40, Robert Haas <robertmhaas@gmail.com> wrote:
..in general I'm
skeptical that we can really set up something that is OK to do in an
aborted transaction, because our ability to handle any further errors
at that point is extremely limited, and this code is definitely
complex enough that it could error out.
Rahila and I were talking about this last week at PGConf.dev and one safety
measure to implement here is to skip this processing for aborted transactions.
While there will be cases when debugging the memory of an aborted transaction
is of interest, we are as you say limited in what we can/should do in that case
so immediately returning is the safer option. The attached also encodes the
example from this thread as a test using an interactive background.
(The attached has a small context id fix as well but the interesting bit is the
above.)
--
Daniel Gustafsson
Attachments:
0001-Avoid-memory-context-reporting-in-aborted-transactio.patchapplication/octet-stream; name=0001-Avoid-memory-context-reporting-in-aborted-transactio.patch; x-unix-mode=0644Download
From 4f2f9faedd05e7d01b9413ee9679f994a24bd86f Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 19 May 2025 21:37:00 +0200
Subject: [PATCH] Avoid memory context reporting in aborted transactions
Author:
Reviewed-by:
Discussion:
---
src/backend/utils/mmgr/mcxt.c | 17 +++-
.../test_misc/t/008_memcxt_reporting.pl | 78 +++++++++++++++++++
2 files changed, 91 insertions(+), 4 deletions(-)
create mode 100644 src/test/modules/test_misc/t/008_memcxt_reporting.pl
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 7d28ca706eb..ee80b552a8b 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -21,6 +21,7 @@
#include "postgres.h"
+#include "access/xact.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "nodes/pg_list.h"
@@ -1450,6 +1451,16 @@ ProcessGetMemoryContextInterrupt(void)
PublishMemoryContextPending = false;
+ /*
+ * Avoid performing any shared memory operations in aborted transaction,
+ * the caller will get the fallback behaviour of the past known stats.
+ */
+ if (IsAbortedTransactionBlockState())
+ {
+ ConditionVariableBroadcast(&memCxtState[idx].memcxt_cv);
+ return;
+ }
+
/*
* The hash table is used for constructing "path" column of the view,
* similar to its local backend counterpart.
@@ -1569,7 +1580,7 @@ ProcessGetMemoryContextInterrupt(void)
if (summary)
{
- int cxt_id = 0;
+ int cxt_id = 1;
List *path = NIL;
/* Copy TopMemoryContext statistics to DSA */
@@ -1577,9 +1588,8 @@ ProcessGetMemoryContextInterrupt(void)
(*TopMemoryContext->methods->stats) (TopMemoryContext, NULL, NULL,
&stat, true);
path = lcons_int(1, path);
- PublishMemoryContext(meminfo, cxt_id, TopMemoryContext, path, stat,
+ PublishMemoryContext(meminfo, 0, TopMemoryContext, path, stat,
1, MemoryStatsDsaArea, 100);
- cxt_id = cxt_id + 1;
/*
* Copy statistics for each of TopMemoryContexts children. This
@@ -1609,7 +1619,6 @@ ProcessGetMemoryContextInterrupt(void)
PublishMemoryContext(meminfo, cxt_id, c, path,
grand_totals, num_contexts, MemoryStatsDsaArea, 100);
}
- memCxtState[idx].total_stats = cxt_id;
/* Notify waiting backends and return */
end_memorycontext_reporting();
diff --git a/src/test/modules/test_misc/t/008_memcxt_reporting.pl b/src/test/modules/test_misc/t/008_memcxt_reporting.pl
new file mode 100644
index 00000000000..f94c3f6374b
--- /dev/null
+++ b/src/test/modules/test_misc/t/008_memcxt_reporting.pl
@@ -0,0 +1,78 @@
+
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Test process memory context reporting, right now we are testing interacting
+# with aborted transactions.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+$node->start;
+
+# Start up a node which we can keep open in an aborted state, and grab the pid
+# from the backend function. If the pid fails to get registered, the tests will
+# still execute but will fail using pid 0.
+my $bsession = $node->interactive_psql('postgres');
+my $bpid = $bsession->query_safe('SELECT pg_backend_pid();');
+my $pid = 0;
+if ($bpid =~ /([0-9]{3,})/)
+{
+ $pid = $1;
+}
+
+# First get a memory context report from the backend in a good state
+my $pre = $node->safe_psql('postgres',
+ "SELECT pg_get_process_memory_contexts($pid,false,0.5);");
+
+# Then set the backend in an aborted state and retry the process, it should
+# return the previous report due to it being aborted.
+$bsession->query('BEGIN;');
+$bsession->query('SELECT 1/0;');
+my $post = $node->safe_psql('postgres',
+ "SELECT pg_get_process_memory_contexts($pid,false,0.5);");
+ok($pre eq $post, "Check that aborted backend returned previous stats");
+
+# There should be no errors in the log and the backend should still be in an
+# aborted state.
+$node->log_check("Check for backend exit", 0, log_unlike => [qr/FATAL/]);
+ok($bsession->quit, "Check that aborted backend hadn't terminated");
+
+# Make sure the backend has exited and cannot be queried anymore
+my ($stdout, $stderr, $timed_out);
+$post = $node->psql(
+ 'postgres', "SELECT pg_get_process_memory_contexts($pid,false,0.5);",
+ stdout => \$stdout,
+ stderr => \$stderr);
+ok(!$post, "Check that we didn't get any results");
+ok($stderr =~ /is not a PostgreSQL server process/,
+ "Check STDERR for WARNING");
+
+# Restart and again set the backend in aborted state, this time without any
+# memory context report from it before aborting.
+$bsession = $node->interactive_psql('postgres');
+$bpid = $bsession->query_safe('SELECT pg_backend_pid();');
+$pid = 0;
+if ($bpid =~ /([0-9]{3,})/)
+{
+ $pid = $1;
+}
+$bsession->query('BEGIN;');
+$bsession->query('SELECT 1/0;');
+$post = $node->safe_psql('postgres',
+ "SELECT pg_get_process_memory_contexts($pid,false,0.5);");
+
+# Since there was no fallback we should have a NULL return and not a report.§
+ok( $post eq '',
+ "Check that an aborted backend returns NULL when no previous stats exist"
+);
+ok($bsession->quit, "Check that aborted backend hadn't terminated");
+
+$node->stop;
+
+done_testing();
--
2.39.3 (Apple Git-146)
Robert Haas <robertmhaas@gmail.com> writes:
What I'm concerned about is that I think that (as I said on the other
thread) is that ProcessGetMemoryContextInterrupt is not really at all
safe to execute at an arbitrary CHECK_FOR_INTERRUPTS().
I agree.
In my mind, the possible fixes here are (1) revert that patch, (2)
redesign things so that the problematic code can only get called when
we know that the backend state is sane, or (3) redesign the code so
that it has fewer requirements for correct operation.
I want to argue for reverting, at least for v18. I do not think that
ProcessGetMemoryContextInterrupt is anywhere near release-quality.
I found out while poking into Valgrind leak reports that it leaks
memory --- and does so in TopMemoryContext. This is especially
unfortunate for something that's supposed to be used to investigate
memory consumption: a tool that affects the results under
consideration is not a great tool. The way I'd build it is to make
a special-purpose context that is a top-level context in its own
right (ie not a child of TopMemoryContext) and do all the work
therein, then reset or delete that context on the way out.
I also think it's remarkably poor design to have shoved this code
into mcxt.c, which is a very low-level module that has no business
having the #include dependencies that it acquired in 042a66291.
We've put some other stuff in mcxt.c that probably shouldn't be
there either, but bloating the file by 40% is a bit much.
regards, tom lane
On 20 May 2025, at 04:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I want to argue for reverting, at least for v18. I do not think that
ProcessGetMemoryContextInterrupt is anywhere near release-quality.
I found out while poking into Valgrind leak reports that it leaks
memory --- and does so in TopMemoryContext.
Ugh, that's indeed a showstopper. Attached is a revert of the feature, I want
to re-read and re-test before pushing to make sure I didn't miss anything but
can go ahead with it later today.
--
Daniel Gustafsson
Attachments:
0001-Revert-function-to-get-memory-context-stats-for-proc.patchapplication/octet-stream; name=0001-Revert-function-to-get-memory-context-stats-for-proc.patch; x-unix-mode=0644Download
From 3cae1b7699133d47d33596037db5f4799bd82bd2 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Tue, 20 May 2025 09:50:27 +0200
Subject: [PATCH] Revert function to get memory context stats for processes
Due to concerns raised about the approach, and memory leaks found
in sensitive contexts the functionality is reverted. This reverts
commits 45e7e8ca9, f8c115a6c, d2a1ed172, 55ef7abf8 and 042a66291.
Discussion: https://postgr.es/m/594293.1747708165@sss.pgh.pa.us
---
doc/src/sgml/func.sgml | 171 -----
doc/src/sgml/release-18.sgml | 15 -
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 -
.../utils/activity/wait_event_names.txt | 1 -
src/backend/utils/adt/mcxtfuncs.c | 423 +-----------
src/backend/utils/adt/pg_locale.c | 1 +
src/backend/utils/init/globals.c | 1 -
src/backend/utils/init/postinit.c | 7 -
src/backend/utils/mb/mbutils.c | 1 +
src/backend/utils/mmgr/mcxt.c | 648 +-----------------
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 -
29 files changed, 48 insertions(+), 1398 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b405525a465..c67688cbf5f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28663,143 +28663,6 @@ 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>summary</parameter> <type>boolean</type>, <parameter>timeout</parameter> <type>float</type> )
- <returnvalue>setof record</returnvalue>
- ( <parameter>name</parameter> <type>text</type>,
- <parameter>ident</parameter> <type>text</type>,
- <parameter>type</parameter> <type>text</type>,
- <parameter>path</parameter> <type>integer[]</type>,
- <parameter>level</parameter> <type>integer</type>,
- <parameter>total_bytes</parameter> <type>bigint</type>,
- <parameter>total_nblocks</parameter> <type>bigint</type>,
- <parameter>free_bytes</parameter> <type>bigint</type>,
- <parameter>free_chunks</parameter> <type>bigint</type>,
- <parameter>used_bytes</parameter> <type>bigint</type>,
- <parameter>num_agg_contexts</parameter> <type>integer</type>,
- <parameter>stats_timestamp</parameter> <type>timestamptz</type> )
- </para>
- <para>
- This function handles requests to display the memory contexts of a
- <productname>PostgreSQL</productname> process with the specified
- process ID. The function can be used to send requests to backends as
- well as <glossterm linkend="glossary-auxiliary-proc">auxiliary processes</glossterm>.
- </para>
- <para>
- The returned record contains extended statistics per each memory
- context:
- <itemizedlist spacing="compact">
- <listitem>
- <para>
- <parameter>name</parameter> - The name of the memory context.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>ident</parameter> - Memory context ID (if any).
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>type</parameter> - The type of memory context, possible
- values are: AllocSet, Generation, Slab and Bump.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>path</parameter> - Memory contexts are organized in a
- tree model with TopMemoryContext as the root, and all other memory
- contexts as nodes in the tree. The <parameter>path</parameter>
- displays the path from the root to the current memory context. The
- path is limited to 100 children per node, which each node limited
- to a max depth of 100, to preserve memory during reporting. The
- printed path will also be limited to 100 nodes counting from the
- TopMemoryContext.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>level</parameter> - The level in the tree of the current
- memory context.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>total_bytes</parameter> - The total number of bytes
- allocated to this memory context.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>total_nblocks</parameter> - The total number of blocks
- used for the allocated memory.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>free_bytes</parameter> - The amount of free memory in
- this memory context.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>free_chunks</parameter> - The number of chunks that
- <parameter>free_bytes</parameter> corresponds to.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>used_bytes</parameter> - The total number of bytes
- currently occupied.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>num_agg_contexts</parameter> - The number of memory
- contexts aggregated in the displayed statistics.
- </para>
- </listitem>
- <listitem>
- <para>
- <parameter>stats_timestamp</parameter> - When the statistics were
- extracted from the process.
- </para>
- </listitem>
- </itemizedlist>
- </para>
- <para>
- When <parameter>summary</parameter> is <literal>true</literal>, statistics
- for memory contexts at levels 1 and 2 are displayed, with level 1
- representing the root node (i.e., <literal>TopMemoryContext</literal>).
- Statistics for contexts on level 2 and below are aggregates of all
- child contexts' statistics, where <literal>num_agg_contexts</literal>
- indicate the number aggregated child contexts. When
- <parameter>summary</parameter> is <literal>false</literal>,
- <literal>the num_agg_contexts</literal> value is <literal>1</literal>,
- indicating that individual statistics are being displayed.
- </para>
- <para>
- Busy processes can delay reporting memory context statistics,
- <parameter>timeout</parameter> specifies the number of seconds
- to wait for updated statistics. <parameter>timeout</parameter> can be
- specified in fractions of a second.
- </para>
- <para>
- After receiving memory context statistics from the target process, it
- returns the results as one row per context. If all the contexts don't
- fit within the pre-determined size limit, the remaining context
- statistics are aggregated and a cumulative total is displayed. The
- <literal>num_agg_contexts</literal> column indicates the number of
- contexts aggregated in the displayed statistics. When
- <literal>num_agg_contexts</literal> is <literal>1</literal> it means
- that the context statistics are displayed separately.
- </para></entry>
- </row>
-
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
@@ -28939,40 +28802,6 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560
because it may generate a large number of log messages.
</para>
- <para>
- <function>pg_get_process_memory_contexts</function> can be used to request
- memory contexts statistics of any <productname>PostgreSQL</productname>
- process. For example:
-<programlisting>
-postgres=# SELECT * FROM pg_get_process_memory_contexts(
- (SELECT pid FROM pg_stat_activity
- WHERE backend_type = 'checkpointer'),
- false, 0.5) LIMIT 1;
--[ RECORD 1 ]----+------------------------------
-name | TopMemoryContext
-ident |
-type | AllocSet
-path | {1}
-level | 1
-total_bytes | 90304
-total_nblocks | 3
-free_bytes | 2880
-free_chunks | 1
-used_bytes | 87424
-num_agg_contexts | 1
-stats_timestamp | 2025-03-24 13:55:47.796698+01
-</programlisting>
- <note>
- <para>
- While <function>pg_get_process_memory_contexts</function> can be used to
- query memory contexts of the local backend,
- <structname>pg_backend_memory_contexts</structname>
- (see <xref linkend="view-pg-backend-memory-contexts"/> for more details)
- will be less resource intensive when only the local backend is of interest.
- </para>
- </note>
- </para>
-
</sect2>
<sect2 id="functions-admin-backup">
diff --git a/doc/src/sgml/release-18.sgml b/doc/src/sgml/release-18.sgml
index cdf47ac6d2a..f93c34a94b2 100644
--- a/doc/src/sgml/release-18.sgml
+++ b/doc/src/sgml/release-18.sgml
@@ -984,21 +984,6 @@ This is true even if the tables in different schemas have different column names
</para>
</listitem>
-<!--
-Author: Daniel Gustafsson <dgustafsson@postgresql.org>
-2025-04-08 [042a66291] Add function to get memory context stats for processes
-Author: Daniel Gustafsson <dgustafsson@postgresql.org>
-2025-04-08 [c57971034] Rename argument in pg_get_process_memory_contexts().
--->
-
-<listitem>
-<para>
-Add function pg_get_process_memory_contexts() to report process memory context statistics (Rahila Syed)
-<ulink url="&commit_baseurl;042a66291">§</ulink>
-<ulink url="&commit_baseurl;c57971034">§</ulink>
-</para>
-</listitem>
-
<!--
Author: David Rowley <drowley@postgresql.org>
2024-07-01 [12227a1d5] Add context type field to pg_backend_memory_contexts
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 15efb02badb..08f780a2e63 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -674,11 +674,6 @@ GRANT SELECT ON pg_backend_memory_contexts TO pg_read_all_stats;
REVOKE EXECUTE ON FUNCTION pg_get_backend_memory_contexts() FROM PUBLIC;
GRANT EXECUTE ON FUNCTION pg_get_backend_memory_contexts() TO pg_read_all_stats;
-REVOKE EXECUTE ON FUNCTION
- pg_get_process_memory_contexts(integer, boolean, float) FROM PUBLIC;
-GRANT EXECUTE ON FUNCTION
- pg_get_process_memory_contexts(integer, boolean, float) TO pg_read_all_stats;
-
-- Statistics views
CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 4d4a1a3197e..6bcd797c36a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -781,10 +781,6 @@ ProcessAutoVacLauncherInterrupts(void)
if (LogMemoryContextPending)
ProcessLogMemoryContextInterrupt();
- /* Publish memory contexts of this process */
- if (PublishMemoryContextPending)
- ProcessGetMemoryContextInterrupt();
-
/* Process sinval catchup interrupts that happened while sleeping */
ProcessCatchupInterrupt();
}
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index d3cb3f1891c..fda91ffd1ce 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -663,10 +663,6 @@ ProcessCheckpointerInterrupts(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 f24f574e748..0ae9bf906ec 100644
--- a/src/backend/postmaster/interrupt.c
+++ b/src/backend/postmaster/interrupt.c
@@ -48,10 +48,6 @@ ProcessMainLoopInterrupts(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 cb7408acf4c..7e622ae4bd2 100644
--- a/src/backend/postmaster/pgarch.c
+++ b/src/backend/postmaster/pgarch.c
@@ -867,10 +867,6 @@ ProcessPgArchInterrupts(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 7149a67fcbc..27e86cf393f 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -192,10 +192,6 @@ ProcessStartupProcInterrupts(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 c7a76711cc5..0fec4f1f871 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -879,10 +879,6 @@ ProcessWalSummarizerInterrupts(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 00c76d05356..2fa045e6b0f 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -51,7 +51,6 @@
#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;
@@ -151,7 +150,6 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, InjectionPointShmemSize());
size = add_size(size, SlotSyncShmemSize());
size = add_size(size, AioShmemSize());
- size = add_size(size, MemoryContextReportingShmemSize());
/* include additional requested shmem from preload libraries */
size = add_size(size, total_addin_request);
@@ -345,7 +343,6 @@ CreateOrAttachShmemStructs(void)
WaitEventCustomShmemInit();
InjectionPointShmemInit();
AioShmemInit();
- MemoryContextReportingShmemInit();
}
/*
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index ce69e26d720..a9bb540b55a 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -691,9 +691,6 @@ 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/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 5148ef982e3..46f44bc4511 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -178,8 +178,6 @@ static const char *const BuiltinTrancheNames[] = {
[LWTRANCHE_XACT_SLRU] = "XactSLRU",
[LWTRANCHE_PARALLEL_VACUUM_DSA] = "ParallelVacuumDSA",
[LWTRANCHE_AIO_URING_COMPLETION] = "AioUringCompletion",
- [LWTRANCHE_MEMORY_CONTEXT_REPORTING_STATE] = "MemoryContextReportingState",
- [LWTRANCHE_MEMORY_CONTEXT_REPORTING_PROC] = "MemoryContextReportingPerProcess",
};
StaticAssertDecl(lengthof(BuiltinTrancheNames) ==
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index f194e6b3dcc..e9ef0fbfe32 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -50,7 +50,6 @@
#include "storage/procsignal.h"
#include "storage/spin.h"
#include "storage/standby.h"
-#include "utils/memutils.h"
#include "utils/timeout.h"
#include "utils/timestamp.h"
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1ae51b1b391..6cb8f871834 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3535,9 +3535,6 @@ ProcessInterrupts(void)
if (LogMemoryContextPending)
ProcessLogMemoryContextInterrupt();
- if (PublishMemoryContextPending)
- ProcessGetMemoryContextInterrupt();
-
if (ParallelApplyMessagePending)
ProcessParallelApplyMessages();
}
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 930321905f1..5d9e04d6823 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -161,7 +161,6 @@ 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_CXT_PUBLISH "Waiting for a process to publish memory information."
ABI_compatibility:
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index 7ec2c225016..8e8ce9e2582 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -22,20 +22,26 @@
#include "miscadmin.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
-struct MemoryStatsBackendState *memCxtState = NULL;
-struct MemoryStatsCtl *memCxtArea = NULL;
+
+/*
+ * MemoryContextId
+ * Used for storage of transient identifiers for
+ * pg_get_backend_memory_contexts.
+ */
+typedef struct MemoryContextId
+{
+ MemoryContext context;
+ int context_id;
+} MemoryContextId;
/*
* int_list_to_array
@@ -86,7 +92,7 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
*/
for (MemoryContext cur = context; cur != NULL; cur = cur->parent)
{
- MemoryStatsContextId *entry;
+ MemoryContextId *entry;
bool found;
entry = hash_search(context_id_lookup, &cur, HASH_FIND, &found);
@@ -140,51 +146,36 @@ PutMemoryContextsStatsTupleStore(Tuplestorestate *tupstore,
else
nulls[1] = true;
- type = ContextTypeToString(context->type);
-
- values[2] = CStringGetTextDatum(type);
- values[3] = Int32GetDatum(list_length(path)); /* level */
- values[4] = int_list_to_array(path);
- values[5] = Int64GetDatum(stat.totalspace);
- values[6] = Int64GetDatum(stat.nblocks);
- values[7] = Int64GetDatum(stat.freespace);
- values[8] = Int64GetDatum(stat.freechunks);
- values[9] = Int64GetDatum(stat.totalspace - stat.freespace);
-
- tuplestore_putvalues(tupstore, tupdesc, values, nulls);
- list_free(path);
-}
-
-/*
- * ContextTypeToString
- * Returns a textual representation of a context type
- *
- * This should cover the same types as MemoryContextIsValid.
- */
-const char *
-ContextTypeToString(NodeTag type)
-{
- const char *context_type;
-
- switch (type)
+ switch (context->type)
{
case T_AllocSetContext:
- context_type = "AllocSet";
+ type = "AllocSet";
break;
case T_GenerationContext:
- context_type = "Generation";
+ type = "Generation";
break;
case T_SlabContext:
- context_type = "Slab";
+ type = "Slab";
break;
case T_BumpContext:
- context_type = "Bump";
+ type = "Bump";
break;
default:
- context_type = "???";
+ type = "???";
break;
}
- return context_type;
+
+ values[2] = CStringGetTextDatum(type);
+ values[3] = Int32GetDatum(list_length(path)); /* level */
+ values[4] = int_list_to_array(path);
+ values[5] = Int64GetDatum(stat.totalspace);
+ values[6] = Int64GetDatum(stat.nblocks);
+ values[7] = Int64GetDatum(stat.freespace);
+ values[8] = Int64GetDatum(stat.freechunks);
+ values[9] = Int64GetDatum(stat.totalspace - stat.freespace);
+
+ tuplestore_putvalues(tupstore, tupdesc, values, nulls);
+ list_free(path);
}
/*
@@ -201,7 +192,7 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
HTAB *context_id_lookup;
ctl.keysize = sizeof(MemoryContext);
- ctl.entrysize = sizeof(MemoryStatsContextId);
+ ctl.entrysize = sizeof(MemoryContextId);
ctl.hcxt = CurrentMemoryContext;
context_id_lookup = hash_create("pg_get_backend_memory_contexts",
@@ -228,7 +219,7 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
foreach_ptr(MemoryContextData, cur, contexts)
{
- MemoryStatsContextId *entry;
+ MemoryContextId *entry;
bool found;
/*
@@ -236,8 +227,8 @@ pg_get_backend_memory_contexts(PG_FUNCTION_ARGS)
* PutMemoryContextsStatsTupleStore needs this to populate the "path"
* column with the parent context_ids.
*/
- entry = (MemoryStatsContextId *) hash_search(context_id_lookup, &cur,
- HASH_ENTER, &found);
+ entry = (MemoryContextId *) hash_search(context_id_lookup, &cur,
+ HASH_ENTER, &found);
entry->context_id = context_id++;
Assert(!found);
@@ -317,349 +308,3 @@ 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,
- * wait for the results and display them.
- *
- * By default, only superusers or users with ROLE_PG_READ_ALL_STATS are allowed
- * to signal a process to return the memory contexts. This is because allowing
- * any users to issue this request at an unbounded rate would cause lots of
- * requests to be sent, 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 details
- * to a dynamic shared memory space.
- *
- * We have defined a limit on DSA memory that could be allocated per process -
- * if the process has more memory contexts than what can fit in the allocated
- * size, the excess contexts are summarized and represented as cumulative total
- * at the end of the buffer.
- *
- * After sending the signal, wait on a condition variable. The publishing
- * backend, after copying the data to shared memory, sends signal on that
- * condition variable. There is one condition variable per publishing backend.
- * Once the condition variable is signalled, check if the latest memory context
- * information is available and display.
- *
- * If the publishing backend does not respond before the condition variable
- * times out, which is set to MEMSTATS_WAIT_TIMEOUT, retry given that there is
- * time left within the timeout specified by the user, before giving up and
- * returning previously published statistics, if any. If no previous statistics
- * exist, return NULL.
- */
-#define MEMSTATS_WAIT_TIMEOUT 100
-Datum
-pg_get_process_memory_contexts(PG_FUNCTION_ARGS)
-{
- int pid = PG_GETARG_INT32(0);
- bool summary = PG_GETARG_BOOL(1);
- double timeout = PG_GETARG_FLOAT8(2);
- PGPROC *proc;
- ProcNumber procNumber = INVALID_PROC_NUMBER;
- bool proc_is_aux = false;
- ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
- MemoryStatsEntry *memcxt_info;
- TimestampTz start_timestamp;
-
- /*
- * See if the process with given pid is a backend or an auxiliary process
- * and remember the type for when we requery the process later.
- */
- proc = BackendPidGetProc(pid);
- if (proc == NULL)
- {
- proc = AuxiliaryPidGetProc(pid);
- proc_is_aux = true;
- }
-
- /*
- * BackendPidGetProc() and AuxiliaryPidGetProc() return NULL if the pid
- * isn't valid; this is however not a problem and leave with a WARNING.
- * See comment in pg_log_backend_memory_contexts for a discussion on this.
- */
- 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();
- }
-
- InitMaterializedSRF(fcinfo, 0);
-
- procNumber = GetNumberFromPGProc(proc);
-
- LWLockAcquire(&memCxtState[procNumber].lw_lock, LW_EXCLUSIVE);
- memCxtState[procNumber].summary = summary;
- LWLockRelease(&memCxtState[procNumber].lw_lock);
-
- start_timestamp = GetCurrentTimestamp();
-
- /*
- * Send a signal to a PostgreSQL process, informing it we want it to
- * produce information about its memory contexts.
- */
- if (SendProcSignal(pid, PROCSIG_GET_MEMORY_CONTEXT, procNumber) < 0)
- {
- ereport(WARNING,
- errmsg("could not send signal to process %d: %m", pid));
- PG_RETURN_NULL();
- }
-
- /*
- * Even if the proc has published statistics, the may not be due to the
- * current request, but 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. Wait for the timeout specified by the
- * user, following which display old statistics if available or return
- * NULL.
- */
- while (1)
- {
- long msecs;
-
- /*
- * We expect to come out of sleep when the requested process has
- * finished publishing the statistics, verified using the 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(&memCxtState[procNumber].lw_lock, LW_EXCLUSIVE);
-
- /*
- * Note in procnumber.h file says that a procNumber can be re-used for
- * a different backend immediately after a backend exits. In case an
- * old process' data was there and not updated by the current process
- * in the slot identified by the procNumber, the pid of the requested
- * process and the proc_id might not match.
- */
- if (memCxtState[procNumber].proc_id == pid)
- {
- /*
- * Break if the latest stats have been read, indicated by
- * statistics timestamp being newer than the current request
- * timestamp.
- */
- msecs = TimestampDifferenceMilliseconds(start_timestamp,
- memCxtState[procNumber].stats_timestamp);
-
- if (DsaPointerIsValid(memCxtState[procNumber].memstats_dsa_pointer)
- && msecs > 0)
- break;
- }
- LWLockRelease(&memCxtState[procNumber].lw_lock);
-
- /*
- * Recheck the state of the backend before sleeping on the condition
- * variable to ensure the process is still alive. Only check the
- * relevant process type based on the earlier PID check.
- */
- if (proc_is_aux)
- proc = AuxiliaryPidGetProc(pid);
- else
- proc = BackendPidGetProc(pid);
-
- /*
- * The process ending during memory context processing is not an
- * error.
- */
- if (proc == NULL)
- {
- ereport(WARNING,
- errmsg("PID %d is no longer a PostgreSQL server process",
- pid));
- PG_RETURN_NULL();
- }
-
- msecs = TimestampDifferenceMilliseconds(start_timestamp, GetCurrentTimestamp());
-
- /*
- * If we haven't already exceeded the timeout value, sleep for the
- * remainder of the timeout on the condition variable.
- */
- if (msecs > 0 && msecs < (timeout * 1000))
- {
- /*
- * Wait for the timeout as defined by the user. If no updated
- * statistics are available within the allowed time then display
- * previously published statistics if there are any. If no
- * previous statistics are available then return NULL. The timer
- * is defined in milliseconds since that's what the condition
- * variable sleep uses.
- */
- if (ConditionVariableTimedSleep(&memCxtState[procNumber].memcxt_cv,
- ((timeout * 1000) - msecs), WAIT_EVENT_MEM_CXT_PUBLISH))
- {
- LWLockAcquire(&memCxtState[procNumber].lw_lock, LW_EXCLUSIVE);
- /* Displaying previously published statistics if available */
- if (DsaPointerIsValid(memCxtState[procNumber].memstats_dsa_pointer))
- break;
- else
- {
- LWLockRelease(&memCxtState[procNumber].lw_lock);
- PG_RETURN_NULL();
- }
- }
- }
- else
- {
- LWLockAcquire(&memCxtState[procNumber].lw_lock, LW_EXCLUSIVE);
- /* Displaying previously published statistics if available */
- if (DsaPointerIsValid(memCxtState[procNumber].memstats_dsa_pointer))
- break;
- else
- {
- LWLockRelease(&memCxtState[procNumber].lw_lock);
- PG_RETURN_NULL();
- }
- }
- }
-
- /*
- * We should only reach here with a valid DSA handle, either containing
- * updated statistics or previously published statistics (identified by
- * the timestamp.
- */
- Assert(memCxtArea->memstats_dsa_handle != DSA_HANDLE_INVALID);
- /* Attach to the dsa area if we have not already done so */
- if (MemoryStatsDsaArea == NULL)
- {
- MemoryContext oldcontext = CurrentMemoryContext;
-
- MemoryContextSwitchTo(TopMemoryContext);
- MemoryStatsDsaArea = dsa_attach(memCxtArea->memstats_dsa_handle);
- MemoryContextSwitchTo(oldcontext);
- dsa_pin_mapping(MemoryStatsDsaArea);
- }
-
- /*
- * Backend has finished publishing the stats, project them.
- */
- memcxt_info = (MemoryStatsEntry *)
- dsa_get_address(MemoryStatsDsaArea, memCxtState[procNumber].memstats_dsa_pointer);
-
-#define PG_GET_PROCESS_MEMORY_CONTEXTS_COLS 12
- for (int i = 0; i < memCxtState[procNumber].total_stats; i++)
- {
- ArrayType *path_array;
- int path_length;
- Datum values[PG_GET_PROCESS_MEMORY_CONTEXTS_COLS];
- bool nulls[PG_GET_PROCESS_MEMORY_CONTEXTS_COLS];
- char *name;
- char *ident;
- Datum *path_datum = NULL;
- int *path_int = NULL;
-
- memset(values, 0, sizeof(values));
- memset(nulls, 0, sizeof(nulls));
-
- if (DsaPointerIsValid(memcxt_info[i].name))
- {
- name = (char *) dsa_get_address(MemoryStatsDsaArea, memcxt_info[i].name);
- values[0] = CStringGetTextDatum(name);
- }
- else
- nulls[0] = true;
-
- if (DsaPointerIsValid(memcxt_info[i].ident))
- {
- ident = (char *) dsa_get_address(MemoryStatsDsaArea, memcxt_info[i].ident);
- values[1] = CStringGetTextDatum(ident);
- }
- else
- nulls[1] = true;
-
- values[2] = CStringGetTextDatum(ContextTypeToString(memcxt_info[i].type));
-
- path_length = memcxt_info[i].path_length;
- path_datum = (Datum *) palloc(path_length * sizeof(Datum));
- if (DsaPointerIsValid(memcxt_info[i].path))
- {
- path_int = (int *) dsa_get_address(MemoryStatsDsaArea, 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);
- values[3] = PointerGetDatum(path_array);
- }
- else
- nulls[3] = true;
-
- values[4] = Int32GetDatum(memcxt_info[i].levels);
- values[5] = Int64GetDatum(memcxt_info[i].totalspace);
- values[6] = Int64GetDatum(memcxt_info[i].nblocks);
- values[7] = Int64GetDatum(memcxt_info[i].freespace);
- values[8] = Int64GetDatum(memcxt_info[i].freechunks);
- values[9] = Int64GetDatum(memcxt_info[i].totalspace -
- memcxt_info[i].freespace);
- values[10] = Int32GetDatum(memcxt_info[i].num_agg_stats);
- values[11] = TimestampTzGetDatum(memCxtState[procNumber].stats_timestamp);
-
- tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
- values, nulls);
- }
- LWLockRelease(&memCxtState[procNumber].lw_lock);
-
- ConditionVariableCancelSleep();
-
- PG_RETURN_NULL();
-}
-
-Size
-MemoryContextReportingShmemSize(void)
-{
- Size sz = 0;
- Size TotalProcs = 0;
-
- TotalProcs = add_size(TotalProcs, NUM_AUXILIARY_PROCS);
- TotalProcs = add_size(TotalProcs, MaxBackends);
- sz = add_size(sz, mul_size(TotalProcs, sizeof(MemoryStatsBackendState)));
-
- sz = add_size(sz, sizeof(MemoryStatsCtl));
-
- return sz;
-}
-
-/*
- * Initialize shared memory for displaying memory context statistics
- */
-void
-MemoryContextReportingShmemInit(void)
-{
- bool found;
-
- memCxtArea = (MemoryStatsCtl *)
- ShmemInitStruct("MemoryStatsCtl",
- sizeof(MemoryStatsCtl), &found);
-
- if (!found)
- {
- LWLockInitialize(&memCxtArea->lw_lock, LWTRANCHE_MEMORY_CONTEXT_REPORTING_STATE);
- memCxtArea->memstats_dsa_handle = DSA_HANDLE_INVALID;
- }
-
- memCxtState = (MemoryStatsBackendState *)
- ShmemInitStruct("MemoryStatsBackendState",
- ((MaxBackends + NUM_AUXILIARY_PROCS) * sizeof(MemoryStatsBackendState)),
- &found);
-
- if (found)
- return;
-
- for (int i = 0; i < (MaxBackends + NUM_AUXILIARY_PROCS); i++)
- {
- ConditionVariableInit(&memCxtState[i].memcxt_cv);
- LWLockInitialize(&memCxtState[i].lw_lock, LWTRANCHE_MEMORY_CONTEXT_REPORTING_PROC);
- memCxtState[i].memstats_dsa_pointer = InvalidDsaPointer;
- }
-}
diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c
index a858f27cadc..f5e31c433a0 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -46,6 +46,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
+#include "utils/relcache.h"
#include "utils/syscache.h"
#ifdef WIN32
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 92b0446b80c..d31cb45a058 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -39,7 +39,6 @@ volatile sig_atomic_t TransactionTimeoutPending = false;
volatile sig_atomic_t IdleSessionTimeoutPending = false;
volatile sig_atomic_t ProcSignalBarrierPending = false;
volatile sig_atomic_t LogMemoryContextPending = false;
-volatile sig_atomic_t PublishMemoryContextPending = false;
volatile sig_atomic_t IdleStatsUpdateTimeoutPending = false;
volatile uint32 InterruptHoldoffCount = 0;
volatile uint32 QueryCancelHoldoffCount = 0;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 89d72cdd5ff..c86ceefda94 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -663,13 +663,6 @@ BaseInit(void)
* drop ephemeral slots, which in turn triggers stats reporting.
*/
ReplicationSlotInitialize();
-
- /*
- * The before shmem exit callback frees the DSA memory occupied by the
- * latest memory context statistics that could be published by this proc
- * if requested.
- */
- before_shmem_exit(AtProcExit_memstats_cleanup, 0);
}
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 308016d7763..886ecbad871 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -39,6 +39,7 @@
#include "mb/pg_wchar.h"
#include "utils/fmgrprotos.h"
#include "utils/memutils.h"
+#include "utils/relcache.h"
#include "varatt.h"
/*
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 7d28ca706eb..0d7e96295d3 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -23,11 +23,6 @@
#include "mb/pg_wchar.h"
#include "miscadmin.h"
-#include "nodes/pg_list.h"
-#include "storage/lwlock.h"
-#include "storage/ipc.h"
-#include "utils/dsa.h"
-#include "utils/hsearch.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
#include "utils/memutils_internal.h"
@@ -140,17 +135,6 @@ static const MemoryContextMethods mcxt_methods[] = {
};
#undef BOGUS_MCTX
-/*
- * This is passed to MemoryContextStatsInternal to determine whether
- * to print context statistics or not and where to print them logs or
- * stderr.
- */
-typedef enum PrintDestination
-{
- PRINT_STATS_TO_STDERR = 0,
- PRINT_STATS_TO_LOGS,
- PRINT_STATS_NONE
-} PrintDestination;
/*
* CurrentMemoryContext
@@ -172,31 +156,16 @@ MemoryContext CurTransactionContext = NULL;
/* This is a transient link to the active portal's memory context: */
MemoryContext PortalContext = NULL;
-dsa_area *MemoryStatsDsaArea = NULL;
static void MemoryContextDeleteOnly(MemoryContext context);
static void MemoryContextCallResetCallbacks(MemoryContext context);
static void MemoryContextStatsInternal(MemoryContext context, int level,
int max_level, int max_children,
MemoryContextCounters *totals,
- PrintDestination print_location,
- int *num_contexts);
+ bool print_to_stderr);
static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
const char *stats_string,
bool print_to_stderr);
-static void PublishMemoryContext(MemoryStatsEntry *memcxt_info,
- int curr_id, MemoryContext context,
- List *path,
- MemoryContextCounters stat,
- int num_contexts, dsa_area *area,
- int max_levels);
-static void compute_contexts_count_and_ids(List *contexts, HTAB *context_id_lookup,
- int *stats_count,
- bool summary);
-static List *compute_context_path(MemoryContext c, HTAB *context_id_lookup);
-static void free_memorycontextstate_dsa(dsa_area *area, int total_stats,
- dsa_pointer prev_dsa_pointer);
-static void end_memorycontext_reporting(void);
/*
* You should not do memory allocations within a critical section, because
@@ -862,19 +831,11 @@ MemoryContextStatsDetail(MemoryContext context,
bool print_to_stderr)
{
MemoryContextCounters grand_totals;
- int num_contexts;
- PrintDestination 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, 1, max_level, max_children,
- &grand_totals, print_location, &num_contexts);
+ MemoryContextStatsInternal(context, 0, max_level, max_children,
+ &grand_totals, print_to_stderr);
if (print_to_stderr)
fprintf(stderr,
@@ -909,14 +870,13 @@ MemoryContextStatsDetail(MemoryContext context,
* One recursion level for MemoryContextStats
*
* Print stats for this context if possible, but in any case accumulate counts
- * into *totals (if not NULL). The callers should make sure that print_location
- * is set to PRINT_STATS_TO_STDERR or PRINT_STATS_TO_LOGS or PRINT_STATS_NONE.
+ * into *totals (if not NULL).
*/
static void
MemoryContextStatsInternal(MemoryContext context, int level,
int max_level, int max_children,
MemoryContextCounters *totals,
- PrintDestination print_location, int *num_contexts)
+ bool print_to_stderr)
{
MemoryContext child;
int ichild;
@@ -924,39 +884,10 @@ MemoryContextStatsInternal(MemoryContext context, int level,
Assert(MemoryContextIsValid(context));
/* Examine the context itself */
- switch (print_location)
- {
- case PRINT_STATS_TO_STDERR:
- context->methods->stats(context,
- MemoryContextStatsPrint,
- &level,
- totals, true);
- break;
-
- case PRINT_STATS_TO_LOGS:
- context->methods->stats(context,
- MemoryContextStatsPrint,
- &level,
- totals, false);
- break;
-
- case PRINT_STATS_NONE:
-
- /*
- * Do not print the statistics if print_location is
- * PRINT_STATS_NONE, only compute totals. This is used in
- * reporting of memory context statistics via a sql function. Last
- * parameter is not relevant.
- */
- context->methods->stats(context,
- NULL,
- NULL,
- totals, false);
- break;
- }
-
- /* Increment the context count for each of the recursive call */
- *num_contexts = *num_contexts + 1;
+ context->methods->stats(context,
+ MemoryContextStatsPrint,
+ &level,
+ totals, print_to_stderr);
/*
* Examine children.
@@ -976,7 +907,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
MemoryContextStatsInternal(child, level + 1,
max_level, max_children,
totals,
- print_location, num_contexts);
+ print_to_stderr);
}
}
@@ -995,13 +926,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
child = MemoryContextTraverseNext(child, context);
}
- /*
- * Add the count of children contexts which are traversed in the
- * non-recursive manner.
- */
- *num_contexts = *num_contexts + ichild;
-
- if (print_location == PRINT_STATS_TO_STDERR)
+ if (print_to_stderr)
{
for (int i = 0; i < level; i++)
fprintf(stderr, " ");
@@ -1014,7 +939,7 @@ MemoryContextStatsInternal(MemoryContext context, int level,
local_totals.freechunks,
local_totals.totalspace - local_totals.freespace);
}
- else if (print_location == PRINT_STATS_TO_LOGS)
+ else
ereport(LOG_SERVER_ONLY,
(errhidestmt(true),
errhidecontext(true),
@@ -1355,22 +1280,6 @@ HandleLogMemoryContextInterrupt(void)
/* latch will be set by procsignal_sigusr1_handler */
}
-/*
- * HandleGetMemoryContextInterrupt
- * Handle receipt of an interrupt indicating a request to publish memory
- * contexts statistics.
- *
- * All the actual work is deferred to ProcessGetMemoryContextInterrupt() as
- * this cannot be performed in a signal handler.
- */
-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.
@@ -1408,539 +1317,6 @@ ProcessLogMemoryContextInterrupt(void)
MemoryContextStatsDetail(TopMemoryContext, 100, 100, false);
}
-/*
- * ProcessGetMemoryContextInterrupt
- * Generate information about memory contexts used by the process.
- *
- * Performs a breadth first search on the memory context tree, thus parents
- * statistics are reported before their children in the monitoring function
- * output.
- *
- * Statistics for all the processes are shared via the same dynamic shared
- * area. Statistics written by each process are tracked independently in
- * per-process DSA pointers. These pointers are stored in static shared memory.
- *
- * We calculate maximum number of context's statistics that can be displayed
- * using a pre-determined limit for memory available per process for this
- * utility maximum size of statistics for each context. The remaining context
- * statistics if any are captured as a cumulative total at the end of
- * individual context's statistics.
- *
- * If summary is true, we capture the level 1 and level 2 contexts
- * statistics. For that we traverse the memory context tree recursively in
- * depth first search manner to cover all the children of a parent context, to
- * be able to display a cumulative total of memory consumption by a parent at
- * level 2 and all its children.
- */
-void
-ProcessGetMemoryContextInterrupt(void)
-{
- List *contexts;
- HASHCTL ctl;
- HTAB *context_id_lookup;
- int context_id = 0;
- MemoryStatsEntry *meminfo;
- bool summary = false;
- int max_stats;
- int idx = MyProcNumber;
- int stats_count = 0;
- int stats_num = 0;
- MemoryContextCounters stat;
- int num_individual_stats = 0;
-
- PublishMemoryContextPending = false;
-
- /*
- * The hash table is used for constructing "path" column of the view,
- * similar to its local backend counterpart.
- */
- ctl.keysize = sizeof(MemoryContext);
- ctl.entrysize = sizeof(MemoryStatsContextId);
- ctl.hcxt = CurrentMemoryContext;
-
- context_id_lookup = hash_create("pg_get_remote_backend_memory_contexts",
- 256,
- &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 defined limit */
- max_stats =
- MEMORY_CONTEXT_REPORT_MAX_PER_BACKEND / MAX_MEMORY_CONTEXT_STATS_SIZE;
- LWLockAcquire(&memCxtState[idx].lw_lock, LW_EXCLUSIVE);
- summary = memCxtState[idx].summary;
- LWLockRelease(&memCxtState[idx].lw_lock);
-
- /*
- * Traverse the memory context tree to find total number of contexts. If
- * summary is requested report the total number of contexts at level 1 and
- * 2 from the top. Also, populate the hash table of context ids.
- */
- compute_contexts_count_and_ids(contexts, context_id_lookup, &stats_count,
- summary);
-
- /*
- * Allocate memory in this process's DSA for storing statistics of the
- * memory contexts upto max_stats, for contexts that don't fit within a
- * limit, a cumulative total is written as the last record in the DSA
- * segment.
- */
- stats_num = Min(stats_count, max_stats);
-
- LWLockAcquire(&memCxtArea->lw_lock, LW_EXCLUSIVE);
-
- /*
- * Create a DSA and send handle to the client process after storing the
- * context statistics. If number of contexts exceed a predefined limit
- * (1MB), a cumulative total is stored for such contexts.
- */
- if (memCxtArea->memstats_dsa_handle == DSA_HANDLE_INVALID)
- {
- MemoryContext oldcontext = CurrentMemoryContext;
- dsa_handle handle;
-
- MemoryContextSwitchTo(TopMemoryContext);
-
- MemoryStatsDsaArea = dsa_create(memCxtArea->lw_lock.tranche);
-
- handle = dsa_get_handle(MemoryStatsDsaArea);
- MemoryContextSwitchTo(oldcontext);
-
- dsa_pin_mapping(MemoryStatsDsaArea);
-
- /*
- * Pin the DSA area, this is to make sure the area remains attachable
- * even if the backend that created it exits. This is done so that the
- * statistics are published even if the process exits while a client
- * is waiting. Also, other processes that publish statistics will use
- * the same area.
- */
- dsa_pin(MemoryStatsDsaArea);
-
- /* Set the handle in shared memory */
- memCxtArea->memstats_dsa_handle = handle;
- }
-
- /*
- * If DSA exists, created by another process publishing statistics, attach
- * to it.
- */
- else if (MemoryStatsDsaArea == NULL)
- {
- MemoryContext oldcontext = CurrentMemoryContext;
-
- MemoryContextSwitchTo(TopMemoryContext);
- MemoryStatsDsaArea = dsa_attach(memCxtArea->memstats_dsa_handle);
- MemoryContextSwitchTo(oldcontext);
- dsa_pin_mapping(MemoryStatsDsaArea);
- }
- LWLockRelease(&memCxtArea->lw_lock);
-
- /*
- * Hold the process lock to protect writes to process specific memory. Two
- * processes publishing statistics do not block each other.
- */
- LWLockAcquire(&memCxtState[idx].lw_lock, LW_EXCLUSIVE);
- memCxtState[idx].proc_id = MyProcPid;
-
- if (DsaPointerIsValid(memCxtState[idx].memstats_dsa_pointer))
- {
- /*
- * Free any previous allocations, free the name, ident and path
- * pointers before freeing the pointer that contains them.
- */
- free_memorycontextstate_dsa(MemoryStatsDsaArea, memCxtState[idx].total_stats,
- memCxtState[idx].memstats_dsa_pointer);
- }
-
- /*
- * Assigning total stats before allocating memory so that memory cleanup
- * can run if any subsequent dsa_allocate call to allocate name/ident/path
- * fails.
- */
- memCxtState[idx].total_stats = stats_num;
- memCxtState[idx].memstats_dsa_pointer =
- dsa_allocate0(MemoryStatsDsaArea, stats_num * sizeof(MemoryStatsEntry));
-
- meminfo = (MemoryStatsEntry *)
- dsa_get_address(MemoryStatsDsaArea, memCxtState[idx].memstats_dsa_pointer);
-
- if (summary)
- {
- int cxt_id = 0;
- List *path = NIL;
-
- /* Copy TopMemoryContext statistics to DSA */
- memset(&stat, 0, sizeof(stat));
- (*TopMemoryContext->methods->stats) (TopMemoryContext, NULL, NULL,
- &stat, true);
- path = lcons_int(1, path);
- PublishMemoryContext(meminfo, cxt_id, TopMemoryContext, path, stat,
- 1, MemoryStatsDsaArea, 100);
- cxt_id = cxt_id + 1;
-
- /*
- * Copy statistics for each of TopMemoryContexts children. This
- * includes statistics of at most 100 children per node, with each
- * child node limited to a depth of 100 in its subtree.
- */
- for (MemoryContext c = TopMemoryContext->firstchild; c != NULL;
- c = c->nextchild)
- {
- MemoryContextCounters grand_totals;
- int num_contexts = 0;
-
- path = NIL;
- memset(&grand_totals, 0, sizeof(grand_totals));
-
- MemoryContextStatsInternal(c, 1, 100, 100, &grand_totals,
- PRINT_STATS_NONE, &num_contexts);
-
- path = compute_context_path(c, context_id_lookup);
-
- /*
- * Register the stats entry first, that way the cleanup handler
- * can reach it in case of allocation failures of one or more
- * members.
- */
- memCxtState[idx].total_stats = cxt_id++;
- PublishMemoryContext(meminfo, cxt_id, c, path,
- grand_totals, num_contexts, MemoryStatsDsaArea, 100);
- }
- memCxtState[idx].total_stats = cxt_id;
-
- /* Notify waiting backends and return */
- end_memorycontext_reporting();
-
- hash_destroy(context_id_lookup);
-
- return;
- }
-
- foreach_ptr(MemoryContextData, cur, contexts)
- {
- List *path = NIL;
-
- /*
- * Figure out the transient context_id of this context and each of its
- * ancestors, to compute a path for this context.
- */
- path = compute_context_path(cur, context_id_lookup);
-
- /* Examine the context stats */
- memset(&stat, 0, sizeof(stat));
- (*cur->methods->stats) (cur, NULL, NULL, &stat, true);
-
- /* Account for saving one statistics slot for cumulative reporting */
- if (context_id < (max_stats - 1) || stats_count <= max_stats)
- {
- /* Copy statistics to DSA memory */
- PublishMemoryContext(meminfo, context_id, cur, path, stat, 1, MemoryStatsDsaArea, 100);
- }
- else
- {
- meminfo[max_stats - 1].totalspace += stat.totalspace;
- meminfo[max_stats - 1].nblocks += stat.nblocks;
- meminfo[max_stats - 1].freespace += stat.freespace;
- meminfo[max_stats - 1].freechunks += stat.freechunks;
- }
-
- /*
- * DSA max limit per process is reached, write aggregate of the
- * remaining statistics.
- *
- * We can store contexts from 0 to max_stats - 1. When stats_count is
- * greater than max_stats, we stop reporting individual statistics
- * when context_id equals max_stats - 2. As we use max_stats - 1 array
- * slot for reporting cumulative statistics or "Remaining Totals".
- */
- if (stats_count > max_stats && context_id == (max_stats - 2))
- {
- char *nameptr;
- int namelen = strlen("Remaining Totals");
-
- num_individual_stats = context_id + 1;
- meminfo[max_stats - 1].name = dsa_allocate(MemoryStatsDsaArea, namelen + 1);
- nameptr = dsa_get_address(MemoryStatsDsaArea, meminfo[max_stats - 1].name);
- strlcpy(nameptr, "Remaining Totals", namelen + 1);
- meminfo[max_stats - 1].ident = InvalidDsaPointer;
- meminfo[max_stats - 1].path = InvalidDsaPointer;
- meminfo[max_stats - 1].type = 0;
- }
- context_id++;
- }
-
- /*
- * Statistics are not aggregated, i.e individual statistics reported when
- * stats_count <= max_stats.
- */
- if (stats_count <= max_stats)
- {
- memCxtState[idx].total_stats = context_id;
- }
- /* Report number of aggregated memory contexts */
- else
- {
- meminfo[max_stats - 1].num_agg_stats = context_id -
- num_individual_stats;
-
- /*
- * Total stats equals num_individual_stats + 1 record for cumulative
- * statistics.
- */
- memCxtState[idx].total_stats = num_individual_stats + 1;
- }
-
- /* Notify waiting backends and return */
- end_memorycontext_reporting();
-
- hash_destroy(context_id_lookup);
-}
-
-/*
- * Update timestamp and signal all the waiting client backends after copying
- * all the statistics.
- */
-static void
-end_memorycontext_reporting(void)
-{
- memCxtState[MyProcNumber].stats_timestamp = GetCurrentTimestamp();
- LWLockRelease(&memCxtState[MyProcNumber].lw_lock);
- ConditionVariableBroadcast(&memCxtState[MyProcNumber].memcxt_cv);
-}
-
-/*
- * compute_context_path
- *
- * Append the transient context_id of this context and each of its ancestors
- * to a list, in order to compute a path.
- */
-static List *
-compute_context_path(MemoryContext c, HTAB *context_id_lookup)
-{
- bool found;
- List *path = NIL;
- MemoryContext cur_context;
-
- for (cur_context = c; cur_context != NULL; cur_context = cur_context->parent)
- {
- MemoryStatsContextId *cur_entry;
-
- cur_entry = hash_search(context_id_lookup, &cur_context, HASH_FIND, &found);
-
- if (!found)
- elog(ERROR, "hash table corrupted, can't construct path value");
-
- path = lcons_int(cur_entry->context_id, path);
- }
-
- return path;
-}
-
-/*
- * Return the number of contexts allocated currently by the backend
- * Assign context ids to each of the contexts.
- */
-static void
-compute_contexts_count_and_ids(List *contexts, HTAB *context_id_lookup,
- int *stats_count, bool summary)
-{
- foreach_ptr(MemoryContextData, cur, contexts)
- {
- MemoryStatsContextId *entry;
- bool found;
-
- entry = (MemoryStatsContextId *) hash_search(context_id_lookup, &cur,
- HASH_ENTER, &found);
- Assert(!found);
-
- /*
- * context id starts with 1 so increment the stats_count before
- * assigning.
- */
- 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 (summary)
- {
- entry = (MemoryStatsContextId *) hash_search(context_id_lookup, &c,
- HASH_ENTER, &found);
- Assert(!found);
-
- entry->context_id = ++(*stats_count);
- }
-
- contexts = lappend(contexts, c);
- }
-
- /*
- * In summary mode only the first two level (from top) contexts are
- * displayed.
- */
- if (summary)
- break;
- }
-}
-
-/*
- * PublishMemoryContext
- *
- * Copy the memory context statistics of a single context to a DSA memory
- */
-static void
-PublishMemoryContext(MemoryStatsEntry *memcxt_info, int curr_id,
- MemoryContext context, List *path,
- MemoryContextCounters stat, int num_contexts,
- dsa_area *area, int max_levels)
-{
- const char *ident = context->ident;
- const char *name = context->name;
- int *path_list;
-
- /*
- * To be consistent with logging output, we label dynahash contexts with
- * just the hash table name as with MemoryContextStatsPrint().
- */
- if (context->ident && strncmp(context->name, "dynahash", 8) == 0)
- {
- name = context->ident;
- ident = NULL;
- }
-
- if (name != NULL)
- {
- int namelen = strlen(name);
- char *nameptr;
-
- if (strlen(name) >= MEMORY_CONTEXT_IDENT_SHMEM_SIZE)
- namelen = pg_mbcliplen(name, namelen,
- MEMORY_CONTEXT_IDENT_SHMEM_SIZE - 1);
-
- memcxt_info[curr_id].name = dsa_allocate(area, namelen + 1);
- nameptr = (char *) dsa_get_address(area, memcxt_info[curr_id].name);
- strlcpy(nameptr, name, namelen + 1);
- }
- else
- memcxt_info[curr_id].name = InvalidDsaPointer;
-
- /* Trim and copy the identifier if it is not set to NULL */
- if (ident != NULL)
- {
- int idlen = strlen(context->ident);
- char *identptr;
-
- /*
- * Some identifiers such as SQL query string can be very long,
- * truncate oversize identifiers.
- */
- if (idlen >= MEMORY_CONTEXT_IDENT_SHMEM_SIZE)
- idlen = pg_mbcliplen(ident, idlen,
- MEMORY_CONTEXT_IDENT_SHMEM_SIZE - 1);
-
- memcxt_info[curr_id].ident = dsa_allocate(area, idlen + 1);
- identptr = (char *) dsa_get_address(area, memcxt_info[curr_id].ident);
- strlcpy(identptr, ident, idlen + 1);
- }
- else
- memcxt_info[curr_id].ident = InvalidDsaPointer;
-
- /* Allocate DSA memory for storing path information */
- if (path == NIL)
- memcxt_info[curr_id].path = InvalidDsaPointer;
- else
- {
- int levels = Min(list_length(path), max_levels);
-
- memcxt_info[curr_id].path_length = levels;
- memcxt_info[curr_id].path = dsa_allocate0(area, levels * sizeof(int));
- memcxt_info[curr_id].levels = list_length(path);
- path_list = (int *) dsa_get_address(area, memcxt_info[curr_id].path);
-
- foreach_int(i, path)
- {
- path_list[foreach_current_index(i)] = i;
- if (--levels == 0)
- break;
- }
- }
- memcxt_info[curr_id].type = context->type;
- memcxt_info[curr_id].totalspace = stat.totalspace;
- memcxt_info[curr_id].nblocks = stat.nblocks;
- memcxt_info[curr_id].freespace = stat.freespace;
- memcxt_info[curr_id].freechunks = stat.freechunks;
- memcxt_info[curr_id].num_agg_stats = num_contexts;
-}
-
-/*
- * free_memorycontextstate_dsa
- *
- * Worker for freeing resources from a MemoryStatsEntry. Callers are
- * responsible for ensuring that the DSA pointer is valid.
- */
-static void
-free_memorycontextstate_dsa(dsa_area *area, int total_stats,
- dsa_pointer prev_dsa_pointer)
-{
- MemoryStatsEntry *meminfo;
-
- meminfo = (MemoryStatsEntry *) dsa_get_address(area, prev_dsa_pointer);
- Assert(meminfo != NULL);
- for (int i = 0; i < total_stats; i++)
- {
- if (DsaPointerIsValid(meminfo[i].name))
- dsa_free(area, meminfo[i].name);
-
- if (DsaPointerIsValid(meminfo[i].ident))
- dsa_free(area, meminfo[i].ident);
-
- if (DsaPointerIsValid(meminfo[i].path))
- dsa_free(area, meminfo[i].path);
- }
-
- dsa_free(area, memCxtState[MyProcNumber].memstats_dsa_pointer);
- memCxtState[MyProcNumber].memstats_dsa_pointer = InvalidDsaPointer;
-}
-
-/*
- * Free the memory context statistics stored by this process
- * in DSA area.
- */
-void
-AtProcExit_memstats_cleanup(int code, Datum arg)
-{
- int idx = MyProcNumber;
-
- if (memCxtArea->memstats_dsa_handle == DSA_HANDLE_INVALID)
- return;
-
- LWLockAcquire(&memCxtState[idx].lw_lock, LW_EXCLUSIVE);
-
- if (!DsaPointerIsValid(memCxtState[idx].memstats_dsa_pointer))
- {
- LWLockRelease(&memCxtState[idx].lw_lock);
- return;
- }
-
- /* If the dsa mapping could not be found, attach to the area */
- if (MemoryStatsDsaArea == NULL)
- MemoryStatsDsaArea = 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(MemoryStatsDsaArea, memCxtState[idx].total_stats,
- memCxtState[idx].memstats_dsa_pointer);
-
- dsa_detach(MemoryStatsDsaArea);
- LWLockRelease(&memCxtState[idx].lw_lock);
-}
-
void *
palloc(Size size)
{
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 62beb71da28..37a484147a8 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -8571,16 +8571,6 @@
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 float8',
- proallargtypes => '{int4,bool,float8,text,text,text,_int4,int4,int8,int8,int8,int8,int8,int4,timestamptz}',
- proargmodes => '{i,i,i,o,o,o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{pid, summary, timeout, name, ident, type, path, level, total_bytes, total_nblocks, free_bytes, free_chunks, used_bytes, num_agg_contexts, stats_timestamp}',
- 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 1e59a7f910f..1bef98471c3 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -96,7 +96,6 @@ 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/lwlock.h b/src/include/storage/lwlock.h
index 2b4cbda39a5..08a72569ae5 100644
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -221,8 +221,6 @@ typedef enum BuiltinTrancheIds
LWTRANCHE_XACT_SLRU,
LWTRANCHE_PARALLEL_VACUUM_DSA,
LWTRANCHE_AIO_URING_COMPLETION,
- LWTRANCHE_MEMORY_CONTEXT_REPORTING_STATE,
- LWTRANCHE_MEMORY_CONTEXT_REPORTING_PROC,
LWTRANCHE_FIRST_USER_DEFINED,
} BuiltinTrancheIds;
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 345d5a0ecb1..afeeb1ca019 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -35,7 +35,6 @@ 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 send 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 c0987dca155..8abc26abce2 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -18,9 +18,6 @@
#define MEMUTILS_H
#include "nodes/memnodes.h"
-#include "storage/condition_variable.h"
-#include "storage/lmgr.h"
-#include "utils/dsa.h"
/*
@@ -51,23 +48,6 @@
#define AllocHugeSizeIsValid(size) ((Size) (size) <= MaxAllocHugeSize)
-/*
- * Memory Context reporting size limits.
- */
-
-/* Max length of context name and ident */
-#define MEMORY_CONTEXT_IDENT_SHMEM_SIZE 64
-/* Maximum size (in bytes) of DSA area per process */
-#define MEMORY_CONTEXT_REPORT_MAX_PER_BACKEND ((size_t) (1 * 1024 * 1024))
-
-/*
- * Maximum size per context. Actual size may be lower as this assumes the worst
- * case of deepest path and longest identifiers (name and ident, thus the
- * multiplication by 2). The path depth is limited to 100 like for memory
- * context logging.
- */
-#define MAX_MEMORY_CONTEXT_STATS_SIZE (sizeof(MemoryStatsEntry) + \
- (100 * sizeof(int)) + (2 * MEMORY_CONTEXT_IDENT_SHMEM_SIZE))
/*
* Standard top-level memory contexts.
@@ -339,66 +319,4 @@ pg_memory_is_all_zeros(const void *ptr, size_t len)
return true;
}
-/* Dynamic shared memory state for statistics per context */
-typedef struct MemoryStatsEntry
-{
- dsa_pointer name;
- dsa_pointer ident;
- dsa_pointer path;
- NodeTag type;
- int path_length;
- int levels;
- int64 totalspace;
- int64 nblocks;
- int64 freespace;
- int64 freechunks;
- int num_agg_stats;
-} MemoryStatsEntry;
-
-/*
- * Static shared memory state representing the DSA area created for memory
- * context statistics reporting. A single DSA area is created and used by all
- * the processes, each having its specific DSA allocations for sharing memory
- * statistics, tracked by per backend static shared memory state.
- */
-typedef struct MemoryStatsCtl
-{
- dsa_handle memstats_dsa_handle;
- LWLock lw_lock;
-} MemoryStatsCtl;
-
-/*
- * Per backend static shared memory state for memory context statistics
- * reporting.
- */
-typedef struct MemoryStatsBackendState
-{
- ConditionVariable memcxt_cv;
- LWLock lw_lock;
- int proc_id;
- int total_stats;
- bool summary;
- dsa_pointer memstats_dsa_pointer;
- TimestampTz stats_timestamp;
-} MemoryStatsBackendState;
-
-
-/*
- * Used for storage of transient identifiers for pg_get_backend_memory_contexts
- */
-typedef struct MemoryStatsContextId
-{
- MemoryContext context;
- int context_id;
-} MemoryStatsContextId;
-
-extern PGDLLIMPORT MemoryStatsBackendState *memCxtState;
-extern PGDLLIMPORT MemoryStatsCtl *memCxtArea;
-extern PGDLLIMPORT dsa_area *MemoryStatsDsaArea;
-extern void ProcessGetMemoryContextInterrupt(void);
-extern const char *ContextTypeToString(NodeTag type);
-extern void HandleGetMemoryContextInterrupt(void);
-extern Size MemoryContextReportingShmemSize(void);
-extern void MemoryContextReportingShmemInit(void);
-extern void AtProcExit_memstats_cleanup(int code, Datum arg);
#endif /* MEMUTILS_H */
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index ae17d028ed3..83228cfca29 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -232,22 +232,3 @@ select * from pg_timezone_abbrevs where abbrev = 'LMT';
LMT | @ 7 hours 52 mins 58 secs ago | f
(1 row)
-DO $$
-DECLARE
- bg_writer_pid int;
- r RECORD;
-BEGIN
- SELECT pid from pg_stat_activity where backend_type='background writer'
- INTO bg_writer_pid;
-
- select type, name, ident
- from pg_get_process_memory_contexts(bg_writer_pid, false, 20)
- where path = '{1}' into r;
- RAISE NOTICE '%', r;
- select type, name, ident
- from pg_get_process_memory_contexts(pg_backend_pid(), false, 20)
- where path = '{1}' into r;
- RAISE NOTICE '%', r;
-END $$;
-NOTICE: (AllocSet,TopMemoryContext,)
-NOTICE: (AllocSet,TopMemoryContext,)
diff --git a/src/test/regress/sql/sysviews.sql b/src/test/regress/sql/sysviews.sql
index d0917b6868e..66179f026b3 100644
--- a/src/test/regress/sql/sysviews.sql
+++ b/src/test/regress/sql/sysviews.sql
@@ -101,21 +101,3 @@ select count(distinct utc_offset) >= 24 as ok from pg_timezone_abbrevs;
-- One specific case we can check without much fear of breakage
-- is the historical local-mean-time value used for America/Los_Angeles.
select * from pg_timezone_abbrevs where abbrev = 'LMT';
-
-DO $$
-DECLARE
- bg_writer_pid int;
- r RECORD;
-BEGIN
- SELECT pid from pg_stat_activity where backend_type='background writer'
- INTO bg_writer_pid;
-
- select type, name, ident
- from pg_get_process_memory_contexts(bg_writer_pid, false, 20)
- where path = '{1}' into r;
- RAISE NOTICE '%', r;
- select type, name, ident
- from pg_get_process_memory_contexts(pg_backend_pid(), false, 20)
- where path = '{1}' into r;
- RAISE NOTICE '%', r;
-END $$;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9ea573fae21..a8346cda633 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1674,10 +1674,6 @@ MemoryContextCounters
MemoryContextData
MemoryContextMethodID
MemoryContextMethods
-MemoryStatsBackendState
-MemoryStatsContextId
-MemoryStatsCtl
-MemoryStatsEntry
MemoryStatsPrintFunc
MergeAction
MergeActionState
--
2.39.3 (Apple Git-146)
On 20 May 2025, at 11:56, Daniel Gustafsson <daniel@yesql.se> wrote:
On 20 May 2025, at 04:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I want to argue for reverting, at least for v18. I do not think that
ProcessGetMemoryContextInterrupt is anywhere near release-quality.
I found out while poking into Valgrind leak reports that it leaks
memory --- and does so in TopMemoryContext.Ugh, that's indeed a showstopper. Attached is a revert of the feature, I want
to re-read and re-test before pushing to make sure I didn't miss anything but
can go ahead with it later today.
This has now been done.
--
Daniel Gustafsson