Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vpfiz-00DEDs-0q for pgsql-hackers@arkaria.postgresql.org; Tue, 10 Feb 2026 04:51:45 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.96) (envelope-from ) id 1vpfix-00DlAV-35 for pgsql-hackers@arkaria.postgresql.org; Tue, 10 Feb 2026 04:51:43 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1vpfix-00DlAN-1x for pgsql-hackers@lists.postgresql.org; Tue, 10 Feb 2026 04:51:43 +0000 Received: from mail-pg1-x534.google.com ([2607:f8b0:4864:20::534]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.98.2) (envelope-from ) id 1vpfiu-00000001vtK-2vne for pgsql-hackers@postgresql.org; Tue, 10 Feb 2026 04:51:42 +0000 Received: by mail-pg1-x534.google.com with SMTP id 41be03b00d2f7-c62239decbeso1341561a12.2 for ; Mon, 09 Feb 2026 20:51:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1770699097; x=1771303897; darn=postgresql.org; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=x4ZectwVUoVNe4EWJidqEl7oav0jiAIIeW1w4CNpPho=; b=PzAw9K/Rp1Fu2vqH8YQf+ItN8On6BnTuJ2LZqCDASzxsk08BohtXOfKTvo3zk1KJPc qbYXEDE0XN3Ch8IykpljSSixV5A3k77F3peWBarumqXDJWW2N5qKasZ8z3Do6od8xkyJ i5KC27JBp0nZaTWMT1RPRSLfhZUC4UH7LuNaUjxjklqhLarGUIxSzkhHKT+buu+VSpv3 PgRM1s2eGxyxTj4M+2rzmi8qTSXksFUMQTxHn22BmFyNr0zmuk1JxlvgqxNAy+DJIZsm bzX8FSn0NtUZ3XyPI00jxVV5523CN/z8agavjG9D3hBjv5iOS2GcyWQAqci/EYzgeNfB FP0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770699097; x=1771303897; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=x4ZectwVUoVNe4EWJidqEl7oav0jiAIIeW1w4CNpPho=; b=J6FHInmAh/jAtFTwrwz/HLM65p4+xa2S1e1+0ZktSW7grEGxFlGdKiSFztNDrWRXV+ WthLuf69HbRqb1kwdp6EPICZojb40yUVhn6mfy8JA5aTHyH2rJbRSGxLvTT53xx0DGOG Nf8GfZZzmDW7NoQFXx7U5laJweE6KPH7TE3RU3ARoGnG0JlbQTRADAIQ8NjiPGB+U/zH QkLB61HxYi+VHTkviDgQ59or3P94UblfX3L/ioauWf65k2hZm/Mjn5P+TNlDt3R7JL8r Co/vf2dhtjTQwkifmjiLpgUiwrl6moiiuq9xLhAkRwOVK5VdvYzbslzBhGSKzGBTTwW8 TAHA== X-Forwarded-Encrypted: i=1; AJvYcCW58wDSN0YFDi3WmPrs+8zRcIW8KJiH6OOBmrknvvhyAqq/ATQ9HhPQL5ccHStLHq+OCFiN3mVfl3NqERr3@postgresql.org X-Gm-Message-State: AOJu0YyhZjXRW9j6i85hMnCQnTSTTkDDsSfYtCWRLVwV9/IlW99Ox1oY Vx+zL5+aq6puh5K2OF7staT6DGY6ipobO13FjOrfe92ch3Q8GTavzHXT X-Gm-Gg: AZuq6aI+s/79r+X1E9YLDQ9vkiG5nZ+9gWrgoGkkC0Q+tE6J+wbU0f+Ut3ukzkh4A1a q+PeZoEr2KkRkmsmuvV2/ZWIf9WShGwlYnbfKKqvCfsu1ICJuB1hGUDIDrLP+TGeKJaXqbQpqoi bDt0rzS2ww9b8lpdLh3fv4FWSv3mn5LvERURaSux9T1eXTxWqd0CCxFQghEMJatvP/1EVFQcoIq SS5ufh2Rb2eTTiyCGlaYDRBlH0Qll6Uv14kjHMGLsNQlpeHM2Z/hJPFocE+RWJ3rcwp/gF2ZXxM RuzTC40LfMGf0cH/OE+8Xi1KLkDFsgzrMjrIXvBOqQBXAjjUG8hsS0kQxUgbikydL/9hBDHEcSk UYFNU+MhTkPqI5pGomP5/pOkmlrwZo8YL4UnqY8ffHmLKn854fF8DdfHm3s3ErrKRszJaFOdFIX YYNHq3ZOBYKfrvtOUk4fL0Gd0NmFfPSvk= X-Received: by 2002:a05:6a21:6f01:b0:393:7575:a8d7 with SMTP id adf61e73a8af0-393ad3b6c74mr12561397637.67.1770699097447; Mon, 09 Feb 2026 20:51:37 -0800 (PST) Received: from smtpclient.apple ([45.32.121.103]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-c6dcb5436c5sm10826555a12.15.2026.02.09.20.51.33 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Feb 2026 20:51:36 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.300.41.1.7\)) Subject: Re: Enhancing Memory Context Statistics Reporting From: Chao Li In-Reply-To: Date: Tue, 10 Feb 2026 12:50:53 +0800 Cc: Daniel Gustafsson , torikoshia , PostgreSQL Hackers , Robert Haas Content-Transfer-Encoding: quoted-printable Message-Id: References: <54FAE93D-E156-4684-9352-1CEDE6BD4A1C@yesql.se> <5bxhxniyvjyfldi7yjxcnxkl3i2ghci2grjyeclrbkfqnyhowk@dfkqzbvtbpml> <3eb40b3e-45c7-426a-b7f8-81f7d05a9b53@oss.nttdata.com> <74105887-E4EF-4CA2-9F0A-3C0CE05BF632@yesql.se> <437100f2-dd86-42e6-830c-5ff413a25e8e@oss.nttdata.com> <2a796830-de2d-4030-b480-d673f6cc5d94@eisentraut.org> <02E42650-0C66-4B49-93CA-1B0A5B73E7FD@yesql.se> To: Rahila Syed X-Mailer: Apple Mail (2.3864.300.41.1.7) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk > On Feb 10, 2026, at 08:03, Rahila Syed wrote: >=20 > Hi, >=20 > Please find attached rebased and updated patches that include a few = fixes > suggested by Daniel. >=20 > 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=20 > 4. Fix the TAP test to compare strings correctly. > 5. Added more comments in the TAP test. >=20 > Thank you, > Rahila Syed > = 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=E2=80=99s convenient to = query a server process=E2=80=99 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 =3D 1; + + *num_contexts =3D 0; + context->methods->stats(context, NULL, NULL, totals, false); ``` As the header comment says that =E2=80=9Ctotals should not be NULL=E2=80=9D= , maybe add Assert(total!=3DNULL) to ensure that. 2 - 0001- mcxt.c ``` + *num_contexts =3D 0; + context->methods->stats(context, NULL, NULL, totals, false); + + for (MemoryContext curr =3D context->firstchild; + curr !=3D NULL; + curr =3D 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 =3D *num_contexts + ichild; +} ``` *num_contexts is only initialized to 0, then *num_contexts =3D = *num_contexts + ichild. Looks like the the initialization is = unnecessary, and the assignment can just be *num_contexts =3D 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 =E2=80=9Cpath=E2= =80=9D 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 =3D { + 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 =3D NIL; + MemoryContext cur_context; + + for (cur_context =3D c; cur_context !=3D NULL; cur_context =3D = cur_context->parent) + { + MemoryContextId *cur_entry; + + cur_entry =3D 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 =3D 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 =3D 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 =E2=80=9CIf the process does not respond with memory = contexts statistics in 5 seconds=E2=80=9D, 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 =3D 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/