public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Rahila Syed <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: torikoshia <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Robert Haas <[email protected]>
Subject: Re: Enhancing Memory Context Statistics Reporting
Date: Tue, 10 Feb 2026 12:50:53 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAH2L28vjHHdpoDKfcLT7nt765k_1X=R4qK7etQyf_DDUg-kNoQ@mail.gmail.com>
References: <CAExHW5skNvp265od6XPs0O-RL3cwtgHW3N87Ob0+nByZ=_HzAA@mail.gmail.com>
<[email protected]>
<CAH2L28v9EU4dxKUpvMt_CFAzG72CYusPWxADsgT=cwFJP-fP0A@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAH2L28s+s4JJdPz2RkALzOvXpxXUcmm=fvg-Y6M82g9Cp=bB-w@mail.gmail.com>
<[email protected]>
<5bxhxniyvjyfldi7yjxcnxkl3i2ghci2grjyeclrbkfqnyhowk@dfkqzbvtbpml>
<[email protected]>
<tesneyk3z2dtrjgwlmkw2wbr7e3olwkowlpke6kl463hfhxedb@fyyqsnwjcp4l>
<CAH2L28tp8RMa0CrCgdCJw20vFzeGQMuHkXAoPgYC5JZuXY8_+g@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAH2L28vt16C9xTuK+K7QZvtA3kCNWXOEiT=gEekUw3Xxp9LVQw@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAH2L28sc-rEhyntPLoaC2XUa0ZjS5ka6KzEbuSVxQBBnUYu1KQ@mail.gmail.com>
<CAH2L28vCCgye_+kJt22RAFzZfYbO7ytSrp-hR6-SenBcm_cN+w@mail.gmail.com>
<CAH2L28t=O1k+5wdcP88rgty3OLZisTU72WGH8Dp2MxJjwn7=fw@mail.gmail.com>
<[email protected]>
<CAH2L28sF02nZj5vEL49EsTx+TfYpB3dFgShgn0=W1V-uFZjejw@mail.gmail.com>
<[email protected]>
<CAH2L28uL=Vm9E4Uf=tkg2Ki7_uDuAXORHVyCA7K_L=PAQ2p4sA@mail.gmail.com>
<[email protected]>
<CAH2L28tACFygy9oXbTA8pPQ6KE8uDiO9CgAhku7ERNpdj1YRTw@mail.gmail.com>
<[email protected]>
<CAH2L28t=c_iBj0+LoKX0TSTrNJB2nTgH6MPD7QgRb8kSBZ7HVg@mail.gmail.com>
<CAH2L28v5qXPpfmVLFDcMFL4MMqsKP92FXhW2eosHA5LbatKnrw@mail.gmail.com>
<[email protected]>
<CAH2L28ucG7zLjLRU4m_hCaGfo_UwUyw89OZR9Q8t8YMriyijgw@mail.gmail.com>
<CAH2L28sMK4qyo9xH9Jy7UTj1seZDDi4+ATjSSKTcrfcvk_V=ig@mail.gmail.com>
<[email protected]>
<CAH2L28vK5T_+u4hOvxm+z_9povNPQWMg=ZxtdD00hngY2QibWA@mail.gmail.com>
<CAH2L28tUUf8o0Ec-5d8XPe=hno3A9Ob4nQ7TgrM1Qt_vJ0Oo2Q@mail.gmail.com>
<CAH2L28vF7R08NUo6Sme8LydRaFcKhS6fu0psfxUQoxsVVqJMwA@mail.gmail.com>
<CAH2L28vqKDTogugPnr54hY8k6enP02U_qMBUtnmo7CbA6OfhHQ@mail.gmail.com>
<CA+TgmoZ2QodyjBOrA9OHdqbH9c7na+YRAYU2sey0ofXFM2FBAw@mail.gmail.com>
<CAH2L28u=78SynqRkNn_v0Xo=c7eiaQQ2b5QQ-pSOmMH-z52eqQ@mail.gmail.com>
<CA+TgmoY7B4oM4MP3PTwVYTa3YKOwjCWFLYitY4FqoT5VN+rw7Q@mail.gmail.com>
<CAH2L28umvzm9YwQBa_zrJM0ABC57ZAKPBxi8bD+n=1ADJ3SOZw@mail.gmail.com>
<CAH2L28vjHHdpoDKfcLT7nt765k_1X=R4qK7etQyf_DDUg-kNoQ@mail.gmail.com>
> On Feb 10, 2026, at 08:03, Rahila Syed <[email protected]> wrote:
>
> Hi,
>
> Please find attached rebased and updated patches that include a few fixes
> suggested by Daniel.
>
> Fixes include
> 1. Changed LW_SHARED to LW_EXCLUSIVE when resetting client_keys.
> 2. Replaced calls to MemoryContextReset with MemoryContextDelete since a
> new memory context is created each time.
> 3. Removed leftover code from earlier versions in ipci.c
> 4. Fix the TAP test to compare strings correctly.
> 5. Added more comments in the TAP test.
>
> Thank you,
> Rahila Syed
> <v50-0001-Add-function-to-report-memory-context-statistics.patch><v50-0002-Test-module-to-test-memory-context-reporting-wit.patch>
Hi Rahila,
Thanks for the patch. I applied v50 locally and played with it a little bit then I reviewed v50. Basically I think it’s convenient to query a server process’ memory usage via a SQL statement.
Here comes my review comments.
1 - 0001- mcxt.c
```
+/*
+ * MemoryContextStatsCounter
+ *
+ * Accumulate statistics counts into *totals. totals should not be NULL.
+ * This involves a non-recursive tree traversal.
+ */
+void
+MemoryContextStatsCounter(MemoryContext context, MemoryContextCounters *totals,
+ int *num_contexts)
+{
+ int ichild = 1;
+
+ *num_contexts = 0;
+ context->methods->stats(context, NULL, NULL, totals, false);
```
As the header comment says that “totals should not be NULL”, maybe add Assert(total!=NULL) to ensure that.
2 - 0001- mcxt.c
```
+ *num_contexts = 0;
+ context->methods->stats(context, NULL, NULL, totals, false);
+
+ for (MemoryContext curr = context->firstchild;
+ curr != NULL;
+ curr = MemoryContextTraverseNext(curr, context))
+ {
+ curr->methods->stats(curr, NULL, NULL, totals, false);
+ ichild++;
+ }
+
+ /*
+ * Add the count of all the children contexts which are traversed
+ * including the parent.
+ */
+ *num_contexts = *num_contexts + ichild;
+}
```
*num_contexts is only initialized to 0, then *num_contexts = *num_contexts + ichild. Looks like the the initialization is unnecessary, and the assignment can just be *num_contexts = ichild.
3 - 0001 - mcxtfuncs.c
```
+typedef struct MemoryStatsEntry
+{
+ char name[MEMORY_CONTEXT_NAME_SHMEM_SIZE];
+ char ident[MEMORY_CONTEXT_IDENT_SHMEM_SIZE];
+ int path[100];
```
You already defined a constant MAX_PATH_DISPLAY_LENGTH below, would it make sense to pull up the macro definition and use it for “path” definition?
4 - 0001 - mctxfuncs.c
```
+/*
+ * Per backend dynamic shared hash entry for memory context statistics
+ * reporting.
+ */
+typedef struct MemoryStatsDSHashEntry
+{
+ char key[64];
+ ConditionVariable memcxt_cv;
+ bool stats_written;
+ int target_server_id;
+ int total_stats;
+ bool summary;
+ dsa_pointer memstats_dsa_pointer;
+} MemoryStatsDSHashEntry;
+
+static const dshash_parameters memctx_dsh_params = {
+ offsetof(MemoryStatsDSHashEntry, memcxt_cv),
+ sizeof(MemoryStatsDSHashEntry),
+ dshash_strcmp,
+ dshash_strhash,
+ dshash_strcpy
+};
```
I wonder why we cannot just use the integer ProcNumber as the hash key? I think dshash fully supports fixed-size binary keys. We can use dshash_memcmp and dshash_memhash for compare and hash functions.
5 - 0001 - mctxfuncs.c
```
+static List *
+compute_context_path(MemoryContext c, HTAB *context_id_lookup)
+{
+ bool found;
+ List *path = NIL;
+ MemoryContext cur_context;
+
+ for (cur_context = c; cur_context != NULL; cur_context = cur_context->parent)
+ {
+ MemoryContextId *cur_entry;
+
+ cur_entry = hash_search(context_id_lookup, &cur_context, HASH_FIND, &found);
+
+ if (!found)
+ {
+ elog(NOTICE, "hash table corrupted, can't construct path value");
+ return NIL;
+ }
+ path = lcons_int(cur_entry->context_id, path);
+ }
+ return path;
+}
```
As in the hash entry, path is a 100-elements array, would it make sense to also limit the path list to not append more than 100 items in this function?
6 - 0001 - mctxfuncs.c
```
+ entry->stats_written = true;
+ dshash_release_lock(MemoryStatsDsHash, entry);
+ hash_destroy(context_id_lookup);
+
+ MemoryContextSwitchTo(oldcontext);
+ MemoryContextDelete(memstats_ctx);
+ /* Notify waiting client backend and return */
+ ConditionVariableSignal(&entry->memcxt_cv);
+}
```
This looks like a race condition. ConditionVariableSignal(&entry->memcxt_cv); is called after the lock is released. So, there is a chance that a process has been terminated, and its before_shmem_exit callback acquired the entry lock and deleted the entry from the hash. So, I think we should send the signal before releasing the lock.
7 - 0001 - mctxfuncs.c
```
+ /*
+ * Wait for MEMORY_STATS_MAX_TIMEOUT. If no statistics are available
+ * within the allowed time then return NULL. The timer is defined in
+ * milliseconds since that's what the condition variable sleep uses.
+ */
+ if (ConditionVariableTimedSleep(&entry->memcxt_cv,
+ (MEMORY_STATS_MAX_TIMEOUT * 1000),
+ WAIT_EVENT_MEM_CXT_PUBLISH))
```
For the wait loop, if the condition wakes up for some other reason, it will loop back and wait for the other 5 seconds, then total wait period will exceed 5 seconds, maybe 9 seconds. This is not a big deal, but the doc explicitly says “If the process does not respond with memory contexts statistics in 5 seconds”, so that behavior might be inconsistent with the doc.
I think we can calculate remaining time and pass remaining milliseconds into ConditionVariableTimedSleep.
8 - 0001 - mctxfuncs.c
```
+ /*
+ * Wait for MEMORY_STATS_MAX_TIMEOUT. If no statistics are available
+ * within the allowed time then return NULL. The timer is defined in
+ * milliseconds since that's what the condition variable sleep uses.
+ */
+ if (ConditionVariableTimedSleep(&entry->memcxt_cv,
+ (MEMORY_STATS_MAX_TIMEOUT * 1000),
+ WAIT_EVENT_MEM_CXT_PUBLISH))
+ {
+ /* Timeout has expired, return NULL */
+ memstats_dsa_cleanup(key);
+ memstats_client_key_reset(procNumber);
+ ConditionVariableCancelSleep();
+ ereport(NOTICE,
+ errmsg("request for memory context statistics for PID %d timed out",
+ pid));
+ PG_RETURN_NULL();
+ }
+ entry = dshash_find_or_insert(MemoryStatsDsHash, key, &found);
+ Assert(found);
```
After ConditionVariableTimedSleep, rather than Assert(found), I think we should check if (!found). Because it waits for 5 seconds without holding the lock, the process may terminate and delete the entry during the period.
9 - 0001 - proc.c
```
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 8560a903bc8..f68583aa820 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -51,6 +51,7 @@
#include "storage/procsignal.h"
#include "storage/spin.h"
#include "storage/standby.h"
+#include "utils/memutils.h"
#include "utils/timeout.h"
#include "utils/timestamp.h"
```
This file is sololy added an include without any other change, that seems unneeded. I tried to remove this include, and the build still passed.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
view thread (40+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Enhancing Memory Context Statistics Reporting
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox