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 <[email protected]> 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 >
v42-0002-Test-module-to-test-memory-context-reporting-with-in.patch
Description: Binary data
v42-0001-Add-function-to-report-memory-context-statistics.patch
Description: Binary data
