Re: Enhancing Memory Context Statistics Reporting

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Rahila Syed <rahilasyed90(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
Subject: Re: Enhancing Memory Context Statistics Reporting
Date: 2026-02-23 09:58:39
Message-ID: BEA4D750-51DC-499A-BF54-BE45FCC10E92@yesql.se
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 21 Feb 2026, at 00:13, Rahila Syed <rahilasyed90(at)gmail(dot)com> 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

Attachment Content-Type Size
v52comments.diff.txt text/plain 10.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message jian he 2026-02-23 10:20:35 Re: NOT NULL NOT ENFORCED
Previous Message John Naylor 2026-02-23 09:40:28 Re: [PATCH] Refactor *_abbrev_convert() functions