MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Mon, 18 Aug 2025 22:32:09 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Rahila Syed <rahilasyed90@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Daniel Gustafsson
 <daniel@yesql.se>
Subject: Re: Enhancing Memory Context Statistics Reporting
In-Reply-To: 
 <CAH2L28sF02nZj5vEL49EsTx+TfYpB3dFgShgn0=W1V-uFZjejw@mail.gmail.com>
References: 
 <CAExHW5skNvp265od6XPs0O-RL3cwtgHW3N87Ob0+nByZ=_HzAA@mail.gmail.com>
 <F23526FA-407E-484B-88BC-4CA81C7CED52@yesql.se>
 <CAH2L28v9EU4dxKUpvMt_CFAzG72CYusPWxADsgT=cwFJP-fP0A@mail.gmail.com>
 <D3689B96-DB1E-4160-AC5E-C2F4E2ACA8CD@yesql.se>
 <54FAE93D-E156-4684-9352-1CEDE6BD4A1C@yesql.se>
 <CAH2L28s+s4JJdPz2RkALzOvXpxXUcmm=fvg-Y6M82g9Cp=bB-w@mail.gmail.com>
 <F50279EA-E138-4C93-BCF5-8B2FC338BFA1@yesql.se>
 <5bxhxniyvjyfldi7yjxcnxkl3i2ghci2grjyeclrbkfqnyhowk@dfkqzbvtbpml>
 <B5BF7FC9-D9AA-4C43-AC28-8A6599056CC0@yesql.se>
 <tesneyk3z2dtrjgwlmkw2wbr7e3olwkowlpke6kl463hfhxedb@fyyqsnwjcp4l>
 <CAH2L28tp8RMa0CrCgdCJw20vFzeGQMuHkXAoPgYC5JZuXY8_+g@mail.gmail.com>
 <F12013F0-F54F-4C3D-92C5-76F8AE944CED@yesql.se>
 <B15D670F-D47B-40DF-A447-0773504B5217@yesql.se>
 <3eb40b3e-45c7-426a-b7f8-81f7d05a9b53@oss.nttdata.com>
 <74105887-E4EF-4CA2-9F0A-3C0CE05BF632@yesql.se>
 <437100f2-dd86-42e6-830c-5ff413a25e8e@oss.nttdata.com>
 <CAH2L28vt16C9xTuK+K7QZvtA3kCNWXOEiT=gEekUw3Xxp9LVQw@mail.gmail.com>
 <2a796830-de2d-4030-b480-d673f6cc5d94@eisentraut.org>
 <02E42650-0C66-4B49-93CA-1B0A5B73E7FD@yesql.se>
 <CAH2L28sc-rEhyntPLoaC2XUa0ZjS5ka6KzEbuSVxQBBnUYu1KQ@mail.gmail.com>
 <CAH2L28vCCgye_+kJt22RAFzZfYbO7ytSrp-hR6-SenBcm_cN+w@mail.gmail.com>
 <CAH2L28t=O1k+5wdcP88rgty3OLZisTU72WGH8Dp2MxJjwn7=fw@mail.gmail.com>
 <e30cdddfbf88d2cc3ccc6c353352f189@oss.nttdata.com>
 <CAH2L28sF02nZj5vEL49EsTx+TfYpB3dFgShgn0=W1V-uFZjejw@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <c31942268d228c69cd457a503de6926b@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/c31942268d228c69cd457a503de6926b%40oss.nttdata.com>
Precedence: bulk

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.