Thread

Re: Enhancing Memory Context Statistics Reporting

Rahila Syed <rahilasyed90@gmail.com> — 2025-11-25T07:20:44Z

Hi Daniel,

Thank you for your comments. Please find attached v41 with all the comments
addressed.


>
> +#include "access/twophase.h"
> +#include "catalog/pg_authid_d.h"
> ...
> +#include "utils/acl.h"
> Are these actually required to be included?
>
>
Removed these.


> -       MemoryContextId *entry;
> +       MemoryStatsContextId *entry;
> Why is this needed?  MemoryStatsContextId is identical to MemoryContextId
> and
> is too only used in mcxtfuncs.c so there is no need to expose it in
> memutils.h.
> Can't you just use MemoryContextId everywhere or am I missing something?
>
>
MemoryContextId has been renamed to MemoryStatsContextId for better
code readability.  I removed the leftover MemoryContextId definition.
Also, I moved it out of memutils.h. Did the same with some other structures
and definitions which were only used in mcxtfuncs.c


> +#define CLIENT_KEY_SIZE 64
> +
> +static LWLock *client_keys_lock = NULL;
> +static int *client_keys = NULL;
> +static dshash_table *MemoryStatsDsHash = NULL;
> +static dsa_area *MemoryStatsDsaArea = NULL;
> These new additions have in some cases too generic names (client_keys etc)
> and
> they all lack comments explaining why they're needed.  Maybe a leading
> block
> comment explaining they are used for process memory context reporting, and
> then
> inline comments on each with their use?
>
>
Added comments.


> +#define CLIENT_KEY_SIZE 64
> ...
> +   char        key[CLIENT_KEY_SIZE];
> ...
> +   snprintf(key, sizeof(key), "%d", MyProcNumber);
> Given that MyProcNumber is an index into the proc array, it seems
> excessive to
> use 64 bytes to store it, can't we get away with a small stack allocation?
>

I agree. Defined it as 32 bytes as MyProcNumber is of size uint32.  Kindly
let me know if you think it can be reduced further.


+    * Retreive the client key for publishing statistics and reset it to -1,
> s/Retreive/Retrieve/
>

Fixed.


> +   ProcNumber  procNumber = INVALID_PROC_NUMBER;
> This variable is never accessed before getting re-assigned, so this
> assignment
> in the variable definition can be removed per project style.
>
>
>
Fixed too.


> +   InitMaterializedSRF(fcinfo, 0);
> Can this initialization be postponed till when we know the ResultSetInfo is
> needed?  While a micro optimization, it seems we can avoid that overhead in
> case the query errors out?
>
>
Good point. Added this just before the result set is getting populated.


> +   if (MemoryStatsDsHash == NULL)
> +       MemoryStatsDsHash =
> GetNamedDSHash("memory_context_statistics_dshash", &memctx_dsh_params,
> &found);
> Nitpick, but there are a few oversize lines, like this one, which need to
> be
> wrapped to match project style.
>
>
I have edited this accordingly.


> +   /*
> +    * XXX. If the process exits without cleaning up its slot, i.e in case
> of
> +    * an abrupt crash the client_keys slot won't be reset thus resulting
> in
> +    * false negative and WARNING would be thrown in case another process
> with
> +    * same slot index is queried for statistics.
> +    */
> +   if (client_keys[procNumber] == -1)
> +       client_keys[procNumber] = MyProcNumber;
> +   else
> +   {
> +       LWLockRelease(client_keys_lock);
> +       ereport(WARNING,
> +               errmsg("server process %d is processing previous request",
> pid));
> +       PG_RETURN_NULL();
> +   }
> AFAICT this mean that a failure to clean up (through a crash for example)
> can
> block a future backend from reporting which isn't entirely ideal.  Is there
> anything we can do to mitigate this?
>
>
Yes, we can reset it when the client times out, as long as we verify that
the value corresponds
to our ProcNumber and not another client's request. Fixed accordingly.


>
> +   bool        summary = false;
> In ProcessGetMemoryContextInterrupt(), can't we just read entry->summary
> rather
> than define a local variable and assign it?  We already read lots of other
> fields from entry directly so it seems more readable to be consistent.
>
>
Fixed.


>
> +   /*
> +    * Add the count of children contexts which are traversed
> +    */
> +   *num_contexts = *num_contexts + ichild;
> Isn't this really the number of children + the parent context?  ichild
> starts
> at one to (AIUI) include the parent context.  Also,
> MemoryContextStatsCounter
> should also make sure to set num_contexts to zero before adding to it.
>
>
Yes. Adjusted the comment to match this and set num_contexts to zero.


>
> +#define MAX_MEMORY_CONTEXT_STATS_SIZE (sizeof(MemoryStatsEntry))
> +#define MAX_MEMORY_CONTEXT_STATS_NUM
> MEMORY_CONTEXT_REPORT_MAX_PER_BACKEND / MAX_MEMORY_CONTEXT_STATS_SIZE
> I don't think MAX_MEMORY_CONTEXT_STATS_SIZE adds any value as it's only
> used
> once, on the line directly after its definition.  We can just use the
> expansion
> of ((sizeof(MemoryStatsEntry)) when defining MAX_MEMORY_CONTEXT_STATS_NUM.
>
>
Fixed.

I've attached the test patch as is, I will clean it up and do further
improvements to it.

Thank you,
Rahila Syed