Re: Enhancing Memory Context Statistics Reporting

On 2025-08-08 18:26, Rahila Syed wrote:
Hi, thanks for working on this again.

> Hi,
> 
> CFbot indicated that the patch requires a rebase, so I've attached an
> updated version.

Here are some comments and questions for v32 patch:

> --- a/doc/src/sgml/func/func-admin.sgml
> +++ b/doc/src/sgml/func/func-admin.sgml
> @@ -251,6 +251,137 @@
>          <literal>false</literal> is returned.
>         </para></entry>
>        </row>
> +
> +      <row>
> +       <entry role="func_table_entry"><para role="func_signature">
> +        <indexterm>
> +         <primary>pg_get_process_memory_contexts</primary>

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.


> +     <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?

   =# select * from pg_get_process_memory_contexts(pg_backend_pid(), 
true, 10);

   -[ RECORD 1 ]----+-----------------------------
   name             | TopMemoryContext
   ident            | [NULL]
   type             | AllocSet
   path             | {1}
   level            | 1
   total_bytes      | 222400
   total_nblocks    | 8
   free_bytes       | 4776
   free_chunks      | 8
   used_bytes       | 217624
   num_agg_contexts | 1


Specifying 0 for timeout causes a crash:

   =# select * from pg_get_process_memory_contexts(74526, true, 0);
   (0 rows)

   =# select 1;
   WARNING:  terminating connection because of crash of another server 
process
   DETAIL:  The postmaster has commanded this server process to roll back 
the current transaction and exit, because another server process exited 
abnormally and possibly corrupted shared memory.
   HINT:  In a moment you should be able to reconnect to the database and 
repeat your command.
   server closed the connection unexpectedly
           This probably means the server terminated abnormally
           before or while processing the request.

Should 0 be handled safely and treated as “no timeout”, or rejected as 
an error?


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().


>     context_id_lookup = 
> hash_create("pg_get_remote_backend_memory_contexts",

> + /* Retreive the client key fo publishing statistics */

fo -> for?

> + * If the publishing backend does not respond before the condition 
> variable
> + * times out, which is set to MEMSTATS_WAIT_TIMEOUT, retry given that 
> there is
> + * time left within the timeout specified by the user, before giving 
> up and
> + * returning previously published statistics, if any. If no previous 
> statistics
> + * exist, return NULL.
> + */
> +#define MEMSTATS_WAIT_TIMEOUT 100

MEMSTATS_WAIT_TIMEOUT is defined, but it doesn’t seem to be used.


-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.