Re: Enhancing Memory Context Statistics Reporting

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.

Thank you,
Rahila Syed