> On 7 Nov 2025, at 23:55, Rahila Syed <[email protected]> wrote:
> I have attached a version 40 patch that has been rebased onto the
> latest master branch, as CFbot indicated a rebase was needed.
Thanks for the rebase, below are a few mostly superficial comments on the
patch:
+#include "access/twophase.h"
+#include "catalog/pg_authid_d.h"
...
+#include "utils/acl.h"
Are these actually required to be included?
- 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?
+#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?
+#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?
+ * Retreive the client key for publishing statistics and reset it to -1,
s/Retreive/Retrieve/
+ 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.
+ 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?
+ 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.
+ /*
+ * 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?
+ 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.
+ /*
+ * 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.
+#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.
> The test module patch is unchanged.
Please include all commits in the series even if they aren't updated since the
CFBot cannot pick them up otherwise.
--
Daniel Gustafsson
