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.