On 2025-08-14 07:35, Rahila Syed wrote:
Hi Torikoshia,
Thank you for reviewing the patch.
This function is added at the end of Table "9.96. Server Signaling
Functions", but since pg_get_process_memory_contexts outputs
essentially
the same information as pg_log_backend_memory_contexts, it might be
better to place them next to each other in the table.
The idea was to place the new addition at the end of the table instead
of in the middle.
I’m fine with putting them together, though. I’ll do that in the
next version unless there’s a
reason not to.
+ <parameter>stats_timestamp</parameter>
<type>timestamptz</type> )
+typedef struct MemoryStatsDSHashEntry
+{
+ char key[64];
+ ConditionVariable memcxt_cv;
+ int proc_id;
+ int total_stats;
+ bool summary;
+ dsa_pointer memstats_dsa_pointer;
+ TimestampTz stats_timestamp;
+} MemoryStatsDSHashEntry;
stats_timestamp appears only in the two places below in the patch,
but
it does not seem to be actually output.
Is this column unnecessary?
Thank you for pointing this out. This is removed in the attached
patch, as it was a
remnant from the previous design. As old statistics are discarded in
the new design,
a timestamp field is not needed anymore.
Specifying 0 for timeout causes a crash:
Should 0 be handled safely and treated as “no timeout”, or
rejected as
an error?
Good catch.
The crash has been resolved in the attached patch. It was caused by a
missing
ConditionVariableCancelSleep() call when exiting without statistics
due to a timeout value of 0.
A 0 timeout means that statistics should only be retrieved if they are
immediately available,
without waiting. We could exit with a warning/error saying "too low
timeout", but I think it's worthwhile
to try fetching the statistics if possible.
Similarly, specifying a negative value for timeout still works:
=# select * from pg_get_process_memory_contexts(30590, true,
-10);
It might be better to reject negative values similar to
pg_terminate_backend().
Fixed as suggested by you in the attached patch.
Currently, negative values are interpreted as an indefinite wait for
statistics.
This could cause the client to hang if the server process exits
without providing statistics.
To avoid this, it would be better to exit after displaying a warning
when the user specifies
negative timeouts.
+ /* Retreive the client key fo publishing statistics */
fo -> for?
Fixed.
+ */
+#define MEMSTATS_WAIT_TIMEOUT 100
MEMSTATS_WAIT_TIMEOUT is defined, but it doesn’t seem to be used.
This is removed now as it was a leftover from the previous design.
The attached patch also fixes an assertion failure I observed when a
client times out
before the last requested process can publish its statistics. A client
frees the memory
reserved for storing the statistics when it exits the function after
timeout. Since a
server process was notified, it might attempt to read the same client
entry and access the dsa
memory reserved for statistics resulting in the assertion
failure. I resolved this by including a check for this scenario and
then exiting the handler
function accordingly.
Thanks for updating the patch!
However, when I ran pg_get_process_memory_contexts() with summary =
true, it took a while and returned nothing:
=# select pg_get_process_memory_contexts(pg_backend_pid(), true, 1)
from pg_stat_activity ;
pg_get_process_memory_contexts
--------------------------------
(0 rows)
Time: 6026.291 ms (00:06.026)
Since v32 patch quickly returned the memory contexts as expected with
the same parameter specified, there seems to be some degradation. Could
you check it?
--
Regards,
--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.
