| 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 |
| 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 |