> On 21 Feb 2026, at 00:13, Rahila Syed <[email protected]> wrote:
> Please find attached updated and rebased patches which incorporate
> code fixes suggested off-list by Daniel
>
> These changes include a fix to free dsa memory in one of the
> exit paths from the client-side function along with several documentation
> and comment improvements.
Thanks for the new version, I think this is looking pretty good now. The
attached .diff.txt (renamed to keep the CFBot from grabbing it) has mostly
whitespace fixes, but also a few small things which are discussed below:
+ <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
Re-reading the docs I think the para's are in the wrong order, as the sentence
above makes little sense to be after the output has already been discussed. I
think this needs to go earlier, see the attached .diff.txt for what I am
proposing.
+ /*
+ * The client process should have created the required DSA and DSHash
+ * table. Here we just attach to those.
+ */
+ if (MemoryStatsDsaArea == NULL)
+ MemoryStatsDsaArea = GetNamedDSA("memory_context_statistics_dsa",
+ &found);
+
+ if (MemoryStatsDsHash == NULL)
+ MemoryStatsDsHash = GetNamedDSHash("memory_context_statistics_dshash",
+ &memctx_dsh_params, &found);
I think this needs an expanded comment discussing why there is no need the
check the DSA and DShash after the GetNamed_ functions, since they can be NULL
going in to this. The attached .diff.txt has a proposal along with an
Assertion to keep future changes of the API from risking subtly breaking this
assumption.
+REGRESS = test_memcontext_reporting
Since the test module doesn't contain any pg_regress style .sql/.out tests so
this line in the Makefile cause a test failure when running with Autoconf.
+$node->append_conf(
+ 'postgresql.conf',
+ qq[
+max_connections = 100
+log_statement = none
+restart_after_crash = false
+]);
Why do we need to set max_connections for this test?
+#Server should have thrown error
+$node->psql(
+ 'postgres',
+ qq(select pg_get_process_memory_contexts($pid, true);),
+ stderr => \$psql_err);
This test doesn't validate that the server actually errored does it? (There is
no proposed fix in the attached.)
--
Daniel Gustafsson
diff --git a/doc/src/sgml/func/func-admin.sgml
b/doc/src/sgml/func/func-admin.sgml
index 120f032f249..aa83b11e131 100644
--- a/doc/src/sgml/func/func-admin.sgml
+++ b/doc/src/sgml/func/func-admin.sgml
@@ -274,10 +274,19 @@
<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>.
- If the process does not respond with memory contexts statistics in 5
seconds,
- the function returns NULL.
+ process ID. The function can be used to send requests to
+ <glossterm linkend="glossary-backend">backends</glossterm> as well as
+ <glossterm linkend="glossary-auxiliary-proc">auxiliary
processes</glossterm>.
+ If the process does not respond with memory contexts statistics in 5
+ seconds, the function returns NULL.
+ </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.
</para>
<para>
The returned record contains extended statistics per each memory
@@ -350,7 +359,8 @@
<listitem>
<para>
<parameter>num_agg_contexts</parameter> - The number of memory
- contexts aggregated in the displayed statistics.
+ contexts aggregated in the displayed statistics.
<literal>1</literal>
+ indicates that context statistics are displayed separately.
</para>
</listitem>
</itemizedlist>
@@ -365,16 +375,6 @@
<parameter>summary</parameter> is <literal>false</literal> (the
default),
<literal>the num_agg_contexts</literal> value is <literal>1</literal>,
indicating that individual statistics are being displayed.
- </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. A
- <literal>num_agg_contexts</literal> value of <literal>1</literal>
- indicates that context statistics are displayed separately.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/utils/adt/mcxtfuncs.c
b/src/backend/utils/adt/mcxtfuncs.c
index f18b6d47c60..fdf0043211b 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -978,7 +978,11 @@ ProcessGetMemoryContextInterrupt(void)
/*
* The client process should have created the required DSA and DSHash
- * table. Here we just attach to those.
+ * table, so we can connect to these directly. The GetNamedXXX
functions
+ * either return DSA/DSHash or error out (which we want to avoid as that
+ * would cause the current transaction to fail) so we don't need to
check
+ * for NULL. An assertion is enough to catch in case this API should be
+ * changed at some point.
*/
if (MemoryStatsDsaArea == NULL)
MemoryStatsDsaArea =
GetNamedDSA("memory_context_statistics_dsa",
@@ -988,6 +992,8 @@ ProcessGetMemoryContextInterrupt(void)
MemoryStatsDsHash =
GetNamedDSHash("memory_context_statistics_dshash",
&memctx_dsh_params, &found);
+ Assert(MemoryStatsDsaArea != NULL && MemoryStatsDsHash != NULL);
+
/*
* The entry lock is held by dshash_find_or_insert to protect writes to
* process specific memory. Two different processes publishing
statistics
@@ -1001,9 +1007,8 @@ ProcessGetMemoryContextInterrupt(void)
* if the caller has timed out waiting for us and have issued a request
to
* another backend.
*
- * Make sure that the client always deletes the entry after taking
- * required lock or this function may end up writing to unallocated
- * memory.
+ * Make sure that the client always deletes the entry after taking the
+ * required lock or this function may end up writing to unallocated
memory.
*/
if (!found || entry->target_server_id != MyProcPid)
{
@@ -1042,13 +1047,11 @@ ProcessGetMemoryContextInterrupt(void)
HASH_ENTER, &found);
Assert(!found);
- /*
- * context id starts with 1
- */
+ /* context id starts with 1 */
contextid_entry->context_id = cxt_id + 1;
/*
- * Copy statistics for each of TopMemoryContexts children. This
+ * Copy statistics for each of TopMemoryContext's children.
This
* includes statistics of at most 100 children per node, with
each
* child node limited to a depth of 100 in its subtree.
*/
diff --git a/src/test/modules/test_memcontext_reporting/Makefile
b/src/test/modules/test_memcontext_reporting/Makefile
index 47bce45bc1d..2ce169ffa69 100644
--- a/src/test/modules/test_memcontext_reporting/Makefile
+++ b/src/test/modules/test_memcontext_reporting/Makefile
@@ -11,8 +11,6 @@ OBJS = \
test_memcontext_reporting.o
PGFILEDESC = "test_memcontext_reporting - test code for memory context
reporting"
-REGRESS = test_memcontext_reporting
-
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
b/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
index 73665abf902..152465c1570 100644
--- a/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
+++ b/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
@@ -21,7 +21,6 @@ $node->init;
$node->append_conf(
'postgresql.conf',
qq[
-max_connections = 100
log_statement = none
restart_after_crash = false
]);
@@ -36,7 +35,7 @@ $node->safe_psql('postgres',
my $pid = $node->safe_psql('postgres',
"SELECT pid from pg_stat_activity where backend_type='checkpointer'");
-#Client should have thrown error
+# Client should have thrown error
$node->psql(
'postgres',
qq(select pg_get_process_memory_contexts($pid, true);),
@@ -44,7 +43,8 @@ $node->psql(
like($psql_err,
qr/error triggered for injection point memcontext-client-injection/);
-#Query the same process after detaching the injection point, using some other
client and it should succeed.
+# Query the same process after detaching the injection point, using some other
+# client and it should succeed.
$node->safe_psql('postgres',
"select injection_points_detach('memcontext-client-injection');");
my $topcontext_name = $node->safe_psql('postgres',
@@ -57,13 +57,14 @@ $node->safe_psql('postgres',
"select injection_points_attach('memcontext-server-injection',
'error');"
);
-#Server should have thrown error
+# Server should have thrown error
$node->psql(
'postgres',
qq(select pg_get_process_memory_contexts($pid, true);),
stderr => \$psql_err);
-#Query the same process after detaching the injection point, using some other
client and it should succeed.
+# Query the same process after detaching the injection point, using some other
+# client and it should succeed.
$node->safe_psql('postgres',
"select injection_points_detach('memcontext-server-injection');");
$topcontext_name = $node->safe_psql('postgres',
@@ -71,8 +72,8 @@ $topcontext_name = $node->safe_psql('postgres',
);
ok($topcontext_name eq 'TopMemoryContext');
-# Test that two concurrent requests to the same process results in a warning
for
-# one of those
+# Test that two concurrent requests to the same process results in a warning
+# for one of those
$node->safe_psql('postgres',
"SELECT injection_points_attach('memcontext-client-injection',
'wait');");
@@ -80,8 +81,9 @@ $node->safe_psql('postgres',
"SELECT injection_points_attach('memcontext-server-wait', 'wait');");
my $psql_session1 = $node->background_psql('postgres');
-# We use query_until so that we can execute additional commands without having
to
-# wait for this command to complete, since it will pause at injection points.
+# We use query_until so that we can execute additional commands without having
+# to wait for this command to complete, since it will pause at injection
+# points.
$psql_session1->query_until(
qr//,
@@ -95,7 +97,7 @@ $node->psql(
stderr => \$psql_err);
ok($psql_err =~
/WARNING: server process $pid is processing previous request/);
-#Wake the client up.
+# Wake the client up.
$node->safe_psql('postgres',
"SELECT injection_points_wakeup('memcontext-client-injection');");
@@ -104,7 +106,8 @@ $node->safe_psql('postgres',
$node->safe_psql('postgres',
"select injection_points_detach('memcontext-server-wait');");
-# Test the client process exiting with timeout does not break the server
process
+# Test the client process exiting with timeout does not break the server
+# process
$node->safe_psql('postgres',
"SELECT injection_points_attach('memcontext-server-wait', 'wait');");
@@ -114,12 +117,12 @@ $node->psql(
qq(select name from pg_get_process_memory_contexts($pid, true) where
path = '{1}';),
stdout => \$psql_out);
is($psql_out, '', 'Query returns null');
-#Wakeup the server process up and detach the injection point.
+# Wakeup the server process up and detach the injection point.
$node->safe_psql('postgres',
"SELECT injection_points_wakeup('memcontext-server-wait');");
$node->safe_psql('postgres',
"select injection_points_detach('memcontext-server-wait');");
-#Query the same server process again and it should succeed.
+# Query the same server process again and it should succeed.
$topcontext_name = $node->safe_psql('postgres',
"select name from pg_get_process_memory_contexts($pid, true) where path
= '{1}';"
);
@@ -130,7 +133,7 @@ $node->safe_psql('postgres',
"select injection_points_attach('memcontext-client-injection',
'test_memcontext_reporting', 'crash', NULL);"
);
-#Client will crash
+# Client will crash
$node->psql(
'postgres',
qq(select name from pg_get_process_memory_contexts($pid, true) where
path = '{1}';),
@@ -143,7 +146,7 @@ like($psql_err,
$node->restart;
$node->poll_query_until('postgres', "SELECT 1;", '1');
-#Querying memory stats should succeed after server start
+# Querying memory stats should succeed after server start
$pid = $node->safe_psql('postgres',
"SELECT pid from pg_stat_activity where backend_type='checkpointer'");
$topcontext_name = $node->safe_psql('postgres',
