public inbox for [email protected]
help / color / mirror / Atom feedFrom: torikoshia <[email protected]>
To: Rahila Syed <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Subject: Re: Enhancing Memory Context Statistics Reporting
Date: Mon, 18 Aug 2025 22:32:09 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAH2L28sF02nZj5vEL49EsTx+TfYpB3dFgShgn0=W1V-uFZjejw@mail.gmail.com>
References: <CAExHW5skNvp265od6XPs0O-RL3cwtgHW3N87Ob0+nByZ=_HzAA@mail.gmail.com>
<[email protected]>
<CAH2L28v9EU4dxKUpvMt_CFAzG72CYusPWxADsgT=cwFJP-fP0A@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAH2L28s+s4JJdPz2RkALzOvXpxXUcmm=fvg-Y6M82g9Cp=bB-w@mail.gmail.com>
<[email protected]>
<5bxhxniyvjyfldi7yjxcnxkl3i2ghci2grjyeclrbkfqnyhowk@dfkqzbvtbpml>
<[email protected]>
<tesneyk3z2dtrjgwlmkw2wbr7e3olwkowlpke6kl463hfhxedb@fyyqsnwjcp4l>
<CAH2L28tp8RMa0CrCgdCJw20vFzeGQMuHkXAoPgYC5JZuXY8_+g@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAH2L28vt16C9xTuK+K7QZvtA3kCNWXOEiT=gEekUw3Xxp9LVQw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAH2L28sc-rEhyntPLoaC2XUa0ZjS5ka6KzEbuSVxQBBnUYu1KQ@mail.gmail.com>
<CAH2L28vCCgye_+kJt22RAFzZfYbO7ytSrp-hR6-SenBcm_cN+w@mail.gmail.com>
<CAH2L28t=O1k+5wdcP88rgty3OLZisTU72WGH8Dp2MxJjwn7=fw@mail.gmail.com>
<[email protected]>
<CAH2L28sF02nZj5vEL49EsTx+TfYpB3dFgShgn0=W1V-uFZjejw@mail.gmail.com>
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.
view thread (52+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Enhancing Memory Context Statistics Reporting
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox