Re: Enhancing Memory Context Statistics Reporting

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.