| From: | Rahila Syed <rahilasyed90(at)gmail(dot)com> |
|---|---|
| To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, torikoshia <torikoshia(at)oss(dot)nttdata(dot)com> |
| Subject: | Re: Enhancing Memory Context Statistics Reporting |
| Date: | 2025-12-18 19:54:42 |
| Message-ID: | CAH2L28tUUf8o0Ec-5d8XPe=hno3A9Ob4nQ7TgrM1Qt_vJ0Oo2Q@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
PFA the updated and rebased patches.
To summarize the work done in this thread,
here are some key concerns discussed in threads [1] and [2] and steps taken
to address those:
*DSA APIs and CFI Handler Safety*: DSA APIs, being high-level, are unsafe
to call from the CFI handler,
which can be invoked from low-level code. This concern was particularly
raised for APIs like `dsa_allocate()`
and `dsa_create()`.
To resolve this, these APIs have been moved out of the CFI handler
function. Now, the dynamic shared memory
needed to store the statistics is allocated and deleted in the client
function. The only operation performed in the CFI
handler is `dsm_attach()`, which attaches to DSA for copying statistics.
Since dsm_attach() only maps the existing
DSM into the current process address space and does not create a new DSM, I
don't see any specific reason why
it would be unsafe to call it from the CFI handler.
*Memory Leak in TopMemoryContext*: A memory leak was reported in the
TopMemoryContext and there were
concerns that memory allocated while executing the memory statistics
reporting function could impact its output.
To address this, we create an exclusive memory context under the NULL
context to handle all memory allocations in
`ProcessGetMemoryContextInterrupt`. This context does not fall under the
TopMemoryContext tree, ensuring that
allocations do not affect the function's outcome. The memory context is
reset at the end of the function, preventing
leaks.
*Error Reported in Thread [2]*: This issue has been fixed by switching to a
NULL resource owner before attaching
to DSM in the CFI handler.
*Other Improvements*:
1. Simplified the user interface by removing the `timeout` argument and
using a constant value instead.
2. Provided a default for the `get_summary` argument so users do not need
to pass a value if they choose not to.
3. The dsm_registry APIs are used to create and attach to DSA and DSHASH
tables, which helps avoid code duplication.
4. Replaced the static shared memory array with a DSHASH table, which holds
metadata such as pointers to memory
containing statistics for each process.
5. The updates previously made to mcxt.c have been moved to mcxtfuncs.c,
which now includes all the existing memory
statistics functions as well as the code for the new proposed function
6. One function that relies on an unexported API is added in mcxt.c
Thank you,
Rahila Syed
[1]. PostgreSQL: Re: pgsql: Add function to get memory context stats for
processes
<https://www.postgresql.org/message-id/CA%2BTgmoaey-kOP1k5FaUnQFd1fR0majVebWcL8ogfLbG_nt-Ytg%40mail.gmail.com>
[2]. PostgreSQL: Re: Prevent an error on attaching/creating a DSM/DSA from
an interrupt handler.
<https://www.postgresql.org/message-id/flat/8B873D49-E0E5-4F9F-B8D6-CA4836B825CD%40yesql.se#7026d2fe4ab0de6dd5decd32eb9c585a>
| Attachment | Content-Type | Size |
|---|---|---|
| v45-0001-Add-function-to-report-memory-context-statistics.patch | application/octet-stream | 57.9 KB |
| v45-0002-Test-module-to-test-memory-context-reporting-wit.patch | application/octet-stream | 8.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Melanie Plageman | 2025-12-18 19:57:57 | Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access) |
| Previous Message | Andres Freund | 2025-12-18 19:49:49 | Re: Qual push down to table AM |