Thread

Re: Enhancing Memory Context Statistics Reporting

Rahila Syed <rahilasyed90@gmail.com> — 2025-11-28T09:22:14Z

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
>