Re: Enhancing Memory Context Statistics Reporting

Hi,

I'm attaching the updated patches, which primarily include cleanup and have
been rebased
following the CFbot report.

Thank you,
Rahila Syed

On Tue, Nov 25, 2025 at 12:50 PM Rahila Syed <rahilasyed90@gmail.com> wrote:

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