Subject: Re: Enhancing Memory Context Statistics Reporting

From: Rahila Syed <[email-masked]>
Date: Thu, 13 Mar 2025 19:26:51 +0530

Hi,

Please find attached updated and rebased patches. It has the following
changes

1. To prevent memory leaks, ensure that the latest statistics published by
a process
are freed before it exits. This can be achieved by calling dsa_free in the
before_shmem_exit callback.
2. Add a level column to maintain consistency with the output of
pg_backend_memory_contexts.

Thank you,
Rahila Syed

On Tue, Mar 4, 2025 at 12:30 PM Rahila Syed <[email-masked]> wrote:

> Hi Daniel,
>
> Thanks for the rebase, a few mostly superficial comments from a first
>> read-through.
>>
> Thank you for your comments.
>
>
>> The documentation refers to attributes in the return row but the format
>> of that
>> row isn't displayed which makes following along hard.  I think we should
>> include a table or a programlisting showing the return data before this
>> paragraph.
>>
>> I included the sql function call and its output in programlisting format
> after the
> function description.
> Since the description was part of a table, I added this additional
> information at the
> end of that table.
>
>
>> +const char *
>> +AssignContextType(NodeTag type)
>> This function doesn't actually assign anything so the name is a bit
>> misleading,
>> it would be better with ContextTypeToString or something similar.
>>
>> Done.
>
>
>>
>> + * By default, only superusers or users with PG_READ_ALL_STATS are
>> allowed to
>> This sentence is really long and should probably be broken up.
>>
>> Fixed.
>
>>
>> + * The shared memory buffer has a limited size - it the process has too
>> many
>> s/it/if/
>>
>> Fixed.
>
>
>> + * If the publishing backend does not respond before the condition
>> variable
>> + * times out, which is set to MEMSTATS_WAIT_TIMEOUT, retry for max_tries
>> + * number of times, which is defined by user, before giving up and
>> + * returning previously published statistics, if any.
>> This comment should mention what happens if the process gives up and
>> there is
>> no previously published stats.
>>
>> Done.
>
>
>>
>> +   int         i;
>>     ...
>> +   for (i = 0; i < memCtxState[procNumber].total_stats; i++)
>> This can be rewritten as "for (int i = 0; .." since we allow C99.
>>
>> Done.
>
>
>>
>> +    * process running and consuming lots of memory, that it might end on
>> its
>> +    * own first and its memory contexts are not logged is not a problem.
>> This comment is copy/pasted from pg_log_backend_memory_contexts and while
>> it
>> mostly still apply it should at least be rewritten to not refer to
>> logging as
>> this function doesn't do that.
>>
>> Fixed.
>
>
>>
>> +   ereport(WARNING,
>> +           (errmsg("PID %d is not a PostgreSQL server process",
>> No need to add the extra parenthesis around errmsg anymore, so I think
>> new code
>> should omit those.
>>
>> Done.
>
>
>>
>> +  errhint("Use pg_backend_memory_contexts view instead")));
>> Super nitpick, but errhints should be complete sentences ending with a
>> period.
>>
>> Done.
>
>
>>
>> +    * statitics have previously been published by the backend. In which
>> case,
>> s/statitics/statistics/
>>
>> Fixed.
>
>
>>
>> +    * statitics have previously been published by the backend. In which
>> case,
>> +    * check if statistics are not older than curr_timestamp, if they are
>> wait
>> I think the sentence around the time check is needlessly confusing, could
>> it be
>> rewritten into something like:
>>     "A valid DSA pointer isn't proof that statistics are available, it
>> can be
>>     valid due to previously published stats.  Check if the stats are
>> updated by
>>     comparing the timestamp, if the stats are newer than our previously
>>     recorded timestamp from before sending the procsignal they must by
>>     definition be updated."
>>
>> Replaced accordingly.
>
>
>>
>> +   /* Assert for dsa_handle to be valid */
>> Was this intended to be turned into an Assert call? Else it seems better
>> to remove.
>>
>
> Added an assert and removed the comment.
>
>
>> +   if (print_location != PRINT_STATS_NONE)
>> This warrants a comment stating why it makes sense.
>>
>
>> +    * Do not print the statistics if print_to_stderr is PRINT_STATS_NONE,
>> s/print_to_stderr/print_location/.  Also, do we really need
>> print_to_stderr in
>> this function at all?  It seems a bit awkward to combine a boolean and a
>> paramter for a tri-state value when the parameter holds the tri_state
>> already.
>> For readability I think just checking print_location will be better since
>> the
>> value will be clear, where print_to_stderr=false is less clear in a
>> tri-state
>> scenario.
>>
>> I removed the boolean print_to_stderr, the checks are now using
> the tri-state enum-print_location.
>
>
>> + * its ancestors to a list, inorder to compute a path.
>> s/inorder/in order/
>>
>> Fixed.
>
>
>>
>> +   elog(LOG, "hash table corrupted, can't construct path value");
>> +   break;
>> This will return either a NIL list or a partial path, but
>> PublishMemoryContext
>> doesn't really take into consideration that it might be so.  Is this
>> really
>> benign to the point that we can blindly go on?  Also, elog(LOG..) is
>> mostly for
>> tracing or debugging as elog() isn't intended for user facing errors.
>>
>> I agree that this should be addressed. I added a check for path value
> before
> storing it in shared memory. If the path is NIL, the path pointer in DSA
> will point
> to InvalidDsaPointer.
> When a client encounters an InvalidDsaPointer it will print NULL in the
> path column.
> Partial path scenario is unlikely IMO, and I am not sure if it warrants
> additional
> checks.
>
>
>> +static void
>> +compute_num_of_contexts(List *contexts, HTAB *context_id_lookup,
>> +                       int *stats_count, bool get_summary)
>> This function does a lot than compute the number of contexts so the name
>> seems
>> a bit misleading.  Perhaps a rename to compute_contexts() or something
>> similar?
>>
>> Renamed to compute_contexts_count_and_ids.
>
>
>>
>> +   memctx_info[curr_id].name = dsa_allocate0(area,
>> +                                             strlen(clipped_ident) + 1);
>> These calls can use idlen instead of more strlen() calls no?  While there
>> is no
>> performance benefit, it would increase readability IMHO if the code first
>> calculates a value, and then use it.
>>
>> Done.
>
>
>>
>> +   strncpy(name,
>> +           clipped_ident, strlen(clipped_ident));
>> Since clipped_ident has been nul terminated earlier there is no need to
>> use
>> strncpy, we can instead use strlcpy and take the target buffer size into
>> consideration rather than the input string length.
>>
>> Replaced with the strlcpy calls.
>
>
>>
>>     PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts
>> */
>> +   PROCSIG_GET_MEMORY_CONTEXT, /* ask backend to log the memory contexts
>> */
>> This comment should be different from the LOG_MEMORY_xx one.
>>
>> Fixed.
>
> +#define MEM_CONTEXT_SHMEM_STATS_SIZE   30
>> +#define MAX_TYPE_STRING_LENGTH 64
>> These are unused, from an earlier version of the patch perhaps?
>>
>> Removed
>
> + * Singe DSA area is created and used by all the processes,
>> s/Singe/Since/
>>
>
> Fixed.
>
> +typedef struct MemoryContextBackendState
>> This is only used in mcxtfuncs.c and can be moved there rather than being
>> exported in the header.
>>
>
> This is being used in mcxt.c too in the form of the variable memCtxState.
>
>
>>
>
> +}          MemoryContextId;
>> This lacks an entry in the typedefs.list file.
>>
>> Added.
>
> Please find attached the updated patches with the above fixes.
>
> Thank you,
> Rahila Syed
>

---

From: Alexander Korotkov <[email-masked]>
Date: Sat, 15 Mar 2025 10:40:39 +0200

Hi, Rahila!

On Thu, Mar 13, 2025 at 3:57 PM Rahila Syed <[email-masked]> wrote:
>
> Please find attached updated and rebased patches. It has the following changes
>
> 1. To prevent memory leaks, ensure that the latest statistics published by a process

Thank you for your work on this subject.

v17-0001-Preparatory-changes-for-reporting-memory-context-sta.patch

It looks like we're increasing *num_contexts twice per child memory
context.  First, it gets increased with a recursive
MemoryContextStatsInternal() call, then by adding an ichild.  I might
be wrong, but I think these calculations at least deserve more
comments.

v17-0002-Function-to-report-memory-context-statistics.patch

+   if (procNumber == MyProcNumber)
+   {
+       ereport(WARNING,
+               errmsg("cannot return statistics for local backend"),
+               errhint("Use pg_backend_memory_contexts view instead."));
+       PG_RETURN_NULL();
+   }

Is it worth it to keep this restriction?  Can we fetch info about
local memory context for the sake of generality?

I know there have been discussions in the thread before, but the
mechanism of publishing memory context stats via DSA looks quite
complicated.  Also, the user probably intends to inspect memory
contexts when there is not a huge amount of free memory.  So, failure
is probable on DSA allocation.  Could we do simpler?  For instance,
allocate some amount of static shared memory and use it as a message
queue between processes.  As a heavy load is not supposed to be here,
I think one queue would be enough.

------
Regards,
Alexander Korotkov
Supabase

---

From: Rahila Syed <[email-masked]>
Date: Mon, 17 Mar 2025 13:22:41 +0530

Hi Alexander,

Thank you for the review.

> be wrong, but I think these calculations at least deserve more

I believe that's not the case. The recursive calls only work for children
encountered up to max_level and less than max_children per context.
The rest of the children are handled using MemoryContextTraverseNext,
without recursive calls. Thus, num_contexts is incremented for those
children separately from the recursive call counter.

I will add more comments around this.

> local memory context for the sake of generality?
I think that could be done, but using pg_backend_memory_context would
be more efficient in this case.

> I think one queue would be enough.
There could be other uses for such a function, such as a monitoring
dashboard
that periodically queries all running backends for memory statistics. If we
use a
single queue shared between all the backends, they will need to wait for
the queue
to become available before sharing their statistics, leading to processing
delays at
the publishing backend.

Even with separate queues for each backend or without expecting concurrent
use,
publishing statistics could be delayed if a message queue is full. This is
because a
backend needs to wait for a client process to consume existing messages or
statistics before publishing more.
If a client process exits without consuming messages, the publishing
backend will
experience timeouts when trying to publish stats. This will impact backend
performance
as statistics are published during CHECK_FOR_INTERRUPTS.

In the current implementation, the backend publishes all the statistics in
one go
without waiting for clients to read any statistics.

In addition, allocating complete message queues in static shared memory can
be
expensive, especially since these static structures need to be created even
if memory
context statistics are never queried.
On the contrary, a dsa is created for the feature whenever statistics are
first queried.
We are not preallocating shared memory for this feature, except for small
structures
to store the dsa_handle and dsa_pointers for each backend.

Thank you,
Rahila Syed

---

From: Ashutosh Bapat <[email-masked]>
Date: Mon, 17 Mar 2025 14:06:50 +0530

> be more efficient in this case.

I have raised a similar concern before. Having two separate functions
one for local backend and other for remote is going to be confusing.
We should have one function doing both and renamed appropriately.

-- 
Best Wishes,
Ashutosh Bapat

---

From: Rahila Syed <[email-masked]>
Date: Thu, 20 Mar 2025 13:09:45 +0530

Hi,

> We should have one function doing both and renamed appropriately.
This is a separate concern from what has been raised by Alexander.
He has suggested removing the restriction and fetching local backend
statistics also
with the proposed function.
I've removed this restriction in the latest version of the patch. Now, the
proposed
function can be used to fetch local backend statistics too.

Regarding your suggestion on merging these functions, although they both
report memory
context statistics, they differ in how they fetch these statistics—locally
versus from dynamic
shared memory. Additionally, the function signatures are different: the
proposed function
takes three arguments (pid, get_summary, and num_tries), whereas
pg_get_backend_memory_contexts does not take any arguments. Therefore, I
believe
these functions can remain separate as long as we document their usages
correctly.

Please find attached rebased and updated patches. I have added some more
comments and
fixed an issue caused due to registering before_shmem_exit callback from
interrupt processing
routine. To address this issue, I am registering this callback in the
InitProcess() function.

This happened because interrupt processing could be triggered from a
PG_ENSURE_ERROR_CLEANUP block. This block operates under the assumption
that
the before_shmem_exit callback registered at the beginning of the block,
will be the last one
in the registered callback list at the end of the block, which would not be
the case if I register
before_shmem_exit callback in the interrupt handling routine.

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 25 Mar 2025 15:14:08 +0100

> On 20 Mar 2025, at 08:39, Rahila Syed <[email-masked]> wrote:

Thanks for the new version, I believe this will be a welcome tool in the
debugging toolbox.

I took a cleanup pass over the docs with among others the below changes:
  * You had broken the text in paragraphs, but without <para/> tags they are
    rendered as a single blob of text so added that.
  * Removed the "(PID)" explanation as process id is used elsewhere on the same
    page already without explanation.
  * Added <productname/> markup on PostgreSQL
  * Added <literal/> markup on paramater values
  * Switched the example query output to use \x
  * Added a note on when pg_backend_memory_contexts is a better choice
The paragraphs need some re-indenting to avoid too long lines, but I opted out
of doing so here to make reviewing the changes easier.

A few comments on the code (all comments are performed in 0003 attached here
which also has smaller cleanups wrt indentation, code style etc):

+#include <math.h>
I don't think we need this, maybe it was from an earlier version of the patch?

+MEM_CTX_PUBLISH    "Waiting for backend to publish memory information."
I wonder if this should really be "process" and not backend?

+       default:
+           context_type = "???";
+           break;
In ContextTypeToString() I'm having second thoughts about this, there shouldn't
be any legitimate use-case of passing a nodetag this function which would fail
MemoryContextIsValid().  I wonder if we aren't helping callers more by erroring
out rather than silently returning an unknown?  I haven't changed this but
maybe we should to set the API right from the start?

+ * if the process has more memory contexts than that can fit in the allocated
s/than that can/than what can/?

+   errmsg("memory context statistics privilege error"));
Similar checks elsewhere in the tree mostly use "permission denied to .." so I
think we should adopt that here as well.

+       LWLockAcquire(&memCtxState[procNumber].lw_lock, LW_EXCLUSIVE);
+       msecs =
+           TimestampDifferenceMilliseconds(curr_timestamp,
+                                           memCtxState[procNumber].stats_timestamp);
Since we only want to consider the stats if they are from the current process,
we can delay checking the time difference until after we've checked the pid and
thus reduce the amount of time we hold the lock in the error case.

+       /*
+        * Recheck the state of the backend before sleeping on the condition
+        * variable
+        */
+       proc = BackendPidGetProc(pid);
Here we are really rechecking that the process is still alive, but I wonder if
we should take the opportunity to ensure that the type is what we expect it to
be?  If the pid has moved from being a backend to an aux proc or vice versa we
really don't want to go on.

+     ereport(WARNING,
+             errmsg("PID %d is not a PostgreSQL server process",
+                    pid));
I wonder if we should differentiate between the warnings?  When we hit this in
the loop the errmsg is describing a slightly different case.  I did leave it
for now, but it's food for thought if we should perhaps reword this one.

+       ereport(LOG,
+               errmsg("Wait for %d process to publish stats timed out, trying again",
+                      pid));
This should probably by DEBUG1, in a congested cluster it might cause a fair
bit of logging which isn't really helping the user.  Also, nitpick, errmsg
starts with a lowercase letter.

+static Size
+MemCtxShmemSize(void)
We don't really need this function anymore and keeping it separate we risk it
going out of sync with the matching calcuation in MemCtxBackendShmemInit, so I
think we should condense into one.

    else
    {
+       Assert(print_location == PRINT_STATS_NONE);
Rather than an if-then-else and an assert we can use a switch statement without
a default, this way we'll automatically get a warning if a value is missed.

+       ereport(LOG,
+               errmsg("hash table corrupted, can't construct path value"));
I know you switched from elog(LOG..  to ereport(LOG..  but I still think a LOG
entry stating corruption isn't helpful, it's not actionable for the user.
Given that it's a case that shouldn't happen I wonder if we should downgrade it
to an Assert(false) and potentially a DEBUG1?

--
Daniel Gustafsson

---

From: Rahila Syed <[email-masked]>
Date: Wed, 26 Mar 2025 16:04:17 +0530

Hi Daniel,

Thank you for your review.

I have incorporated all your changes in v20 patches and ensured that the
review comments
corresponding to 0001 patch are included in that patch and not in 0002.

> maybe we should to set the API right from the start?

I cannot think of any legitimate scenario where the context type would be
unknown.
However, if we were to throw an error, it would prevent us from reporting
any memory
usage information when the context type is unidentified. Perhaps, it would
be more
informative and less restrictive to label it as "Unrecognized" or "Unknown."
I wonder if this was the reasoning behind doing it when it was added with
the
pg_backend_memory_contexts() function.

> really don't want to go on.
The reasoning makes sense to me. For periodic monitoring of all processes,
any PID that reincarnates into a different type could be queried in
subsequent
function calls. Regarding targeted monitoring of a specific process, such a
reincarnated
process would exhibit a completely different memory consumption,
likely not aligning with the user's original intent behind requesting the
statistics.

> for now, but it's food for thought if we should perhaps reword this one.
Changed it to  "PID %d is no longer the same PostgreSQL server process".

> How about changing it to ERROR, in accordance with current occurrences of
the
same message? I did it in the attached version, however I am open to
changing
it to an Assert(false) and DEBUG1.

Apart from the above, I made the following improvements.

1. Eliminated the unnecessary creation of an extra memory context before
calling hash_create.
The hash_create function already generates a memory context containing the
hash table,
enabling easy memory deallocation by simply deleting the context via
hash_destroy.
Therefore, the patch relies on hash_destroy for memory management instead
of manual freeing.

2. Optimized memory usage by storing the path as an array of integers
rather than as an array of
Datums.
This approach conserves DSA memory allocated for storing this information.

3. Miscellaneous comment cleanups and introduced macros to simplify
calculations.

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 2 Apr 2025 23:44:25 +0200

> On 26 Mar 2025, at 11:34, Rahila Syed <[email-masked]> wrote:

> How about changing it to ERROR, in accordance with current occurrences of the
> same message? I did it in the attached version, however I am open to changing

In the attached I moved it to an elog() as it's an internal error, and spending
translation effort on it seems fruitless.

> 1. Eliminated the unnecessary creation of an extra memory context before calling hash_create. 
> The hash_create function already generates a memory context containing the hash table, 
> enabling easy memory deallocation by simply deleting the context via hash_destroy. 
> Therefore, the patch relies on hash_destroy for memory management instead of manual freeing.

Nice

> 2. Optimized memory usage by storing the path as an array of integers rather than as an array of 
> Datums. 

Ah yes, much better.

The attached v21 has a few improvements:

* The function documentation didn't specify the return type, only the fact that
it's setof record.  I've added all output columns.

* Some general cleaups of the docs with better markup, improved xref linking
and various rewording.

* Comment cleanups and language alignment

* Added a missing_ok parameter to ContextTypeToString().  While all callers are
fine with unknown context types, if we introduce an API for this it seems
prudent to not place that burden on callers but to take it on in the function.

* Renamed get_summary to just summary, and num_of_tries to retries which feels
more in line with the naming convention in other functions

* Deferred calling InitMaterializedSRF() until after the PID has been checked
for validity.

* Pulled back the timeout to 500msec from 1 second.  In running congested
pgbench simulations I saw better performance and improved results in getting stats.

* Replaced strncpy with strlcpy and consistently used idlen to keep all length
calculations equal.

* Fixed misspelled param name in pg_proc.dat

* Pulled back maximum memory usage from 8Mb to 1Mb.  8Mb for the duration of a
process (once allocated) is a lot for a niche feature and I while I'm still not
sure 1Mb is the right value I think from experimentation that it's closer.

I think this version is close to a committable state, will spend a little more
time testing, polishing and rewriting the commit message.  I will also play
around with placement within the memory context code files to keep it from
making backpatch issues.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 7 Apr 2025 15:41:37 +0200

Following up on some off-list comments, attached is a v26 with a few small last
changes:

  * Improved documentation (docs and comments)
  * Fixed up Shmem sizing and init
  * Delayed registering to the shmem cleanup to get it earlier in cleanup
  * Renamed a few datastructures to improve readability
  * Various bits of polish

I think this function can be a valuable debugging aid going forward.

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 12:38:28 -0400

Hi,

On 2025-04-07 21:57:57 +0530, Rahila Syed wrote:
> > > diff --git a/src/backend/postmaster/auxprocess.c
> > b/src/backend/postmaster/auxprocess.c
> > > index 4f6795f7265..d3b4df27935 100644
> > > --- a/src/backend/postmaster/auxprocess.c
> > > +++ b/src/backend/postmaster/auxprocess.c
> > > @@ -84,6 +84,13 @@ AuxiliaryProcessMainCommon(void)
> > >       /* register a before-shutdown callback for LWLock cleanup */
> > >       before_shmem_exit(ShutdownAuxiliaryProcess, 0);
> > >
> > > +     /*
> > > +      * The before shmem exit callback frees the DSA memory occupied by
> > the
> > > +      * latest memory context statistics that could be published by
> > this aux
> > > +      * proc if requested.
> > > +      */
> > > +     before_shmem_exit(AtProcExit_memstats_dsa_free, 0);
> > > +
> > >       SetProcessingMode(NormalProcessing);
> > >  }
> >
> > How about putting it into BaseInit()?  Or maybe just register it when its
> > first used?
> >
> >
> Problem with registering it when dsa is first used is that dsa is used in an
> interrupt handler. The handler could be called from the
> PG_ENSURE_ERROR_CLEANUP block. This block operates under the assumption that
> the before_shmem_exit callback registered at the beginning, will be the last
> one in the registered callback list at the end of the block. However, this
> won't be the case if a callback is registered from an interrupt handler
> called in the PG_ENSURE_ERROR_CLEANUP block.

Ugh, I really dislike PG_ENSURE_ERROR_CLEANUP().

That's not an argument against moving it to BaseInit() though, as that's
called before procsignal is even initialized and before signals are unmasked.

> I don't really understand why DSA_DEFAULT_INIT_SEGMENT_SIZE is
> 
> something that makes sense to use here?
> >
> >
> To determine the memory  limit per backend in multiples of
> DSA_DEFAULT_INIT_SEGMENT_SIZE.
> Currently it is set to  1 * DSA_DEFAULT_INIT_SEGMENT_SIZE.
> Since a call to dsa_create would create a DSA segment of this size, I
> thought it makes sense
> to define a limit related to the segment size.

I strongly disagree.  The limit should be in an understandable unit, not on
another subystems's defaults that might change at some point.

> > +     /* If the dsm mapping could not be found, attach to the area */
> > > +     if (dsm_seg != NULL)
> > > +             return;
> >
> > I don't understand what we do here with the dsm?  Why do we not need
> > cleanup
> > if we are already attached to the dsm segment?
> >
> 
> I am not expecting to hit this case, since we are always detaching from the
> dsa.

Pretty sure it's reachable, consider a failure of dsa_allocate(). That'll
throw an error, while attached to the segment.

> This could be an assert but since it is a cleanup code, I thought returning
> would be a harmless step.

The problem is that the code seems wrong - if we are already attached we'll
leak the memory!

As I also mentioned, I don't understand why we're constantly
attaching/detaching from the dsa/dsm either. It just seems to make things more
complicated an dmore expensive.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Mon, 7 Apr 2025 20:03:36 -0400

Hi,

On 2025-04-08 01:17:17 +0200, Daniel Gustafsson wrote:
> > On 7 Apr 2025, at 17:43, Andres Freund <[email-masked]> wrote:
> 
> >> + /*
> >> + * Hold the process lock to protect writes to process specific memory. Two
> >> + * processes publishing statistics do not block each other.
> >> + */
> > 
> > s/specific/process specific/
> 
> That's what it says though.. isn't it? I might be missing something obvious.

Understandable confusion, not sure what my brain was doing anymore
either...

> >> +} MemoryContextState;
> > 
> > IMO that's too generic a name for something in a header.
> > 
> >> +} MemoryContextId;
> > 
> > This too.  Particularly because MemoryContextData->ident exist but is
> > something different.
> 
> Renamed both to use MemoryContextReporting* namespace, which leaves
> MemoryContextReportingBackendState at an unwieldly long name.  I'm running out
> of ideas on how to improve and it does make purpose quite explicit at least.

How about

MemoryContextReportingBackendState -> MemoryStatsBackendState
MemoryContextReportingId -> MemoryStatsContextId
MemoryContextReportingSharedState -> MemoryStatsCtl
MemoryContextReportingStatsEntry -> MemoryStatsEntry

> >> + /* context id starts with 1 */
> >> + entry->context_id = ++(*stats_count);
> > 
> > Given that we don't actually do anything here relating to starting with 1, I
> > find that comment confusing.
> 
> Reworded, not sure if it's much better tbh.

I'd probably just remove the comment.

> > Hm. First I thought we'd leak memory if this second (and subsequent)
> > dsa_allocate failed. Then I thought we'd be ok, because the memory would be
> > memory because it'd be reachable from memCtxState[idx].memstats_dsa_pointer.
> > 
> > But I think it wouldn't *quite* work, because memCtxState[idx].total_stats is
> > only set *after* we would have failed.
> 
> Keeping a running total in .total_stats should make the leak window smaller.

Why not just initialize .total_stats *before* calling any fallible code?
Afaict it's zero-allocated, so the free function should have no problem
dealing with the entries that haven't yet been populated/

Greetings,

Andres Freund

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 8 Apr 2025 10:03:09 +0200

> On 8 Apr 2025, at 07:40, Rahila Syed <[email-masked]> wrote:

> Fixed accordingly.

That's much better, thanks.

There was a bug in the shmem init function which caused it to fail on Windows,
the attached fixes that.

--
Daniel Gustafsson

￼

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 30 Apr 2025 12:43:24 +0200

> On 30 Apr 2025, at 12:14, Peter Eisentraut <[email-masked]> wrote:
> 
> On 29.04.25 15:13, Rahila Syed wrote:
>> Please find attached a patch with some comments and documentation changes.
>> Additionaly, added a missing '\0' termination to "Remaining Totals" string.
>> I think this became necessary after we replaced dsa_allocate0()
>> with dsa_allocate() is the latest version.
> 
> >   strncpy(nameptr, "Remaining Totals", namelen);
> > + nameptr[namelen] = '\0';
> 
> Looks like a case for strlcpy()?

True.  I did go ahead with the strncpy and nul terminator assignment, mostly
out of muscle memory, but I agree that this would be a good place for a
strlcpy() instead.

--
Daniel Gustafsson

---

From: Rahila Syed <[email-masked]>
Date: Fri, 11 Jul 2025 21:01:12 +0530

Hi,

Please find attached the latest memory context statistics monitoring patch.
It has been redesigned to address several issues highlighted in the thread
[1]
and [2].

Here are some key highlights of the new design:

- All DSA processing has been moved out of the CFI handler function. Now,
all the dynamic shared memory
needed to store the statistics is created and deleted in the client
function.  This change addresses concerns
that DSA APIs are too high level to be safely called from interrupt
handlers. There was also a concern that
DSA API calls might not provide re-entrancy, which could cause issues if
CFI is invoked from a DSA function
in the future.

- The static shared memory array has been replaced with a DSHASH table
which now holds metadata such as
pointers to actual statistics for each process.

- dsm_registry.c APIs are used  for creating and attaching to DSA and
DSHASH table, which helps prevent code
duplication.

-To address the memory leak concern, we create an exclusive memory context
under the NULL context, which
does not fall under the TopMemoryContext tree, to handle all the memory
allocations in ProcessGetMemoryContextInterrupt.
This ensures the memory context created by the function does not affect its
outcome.
The memory context is reset at the end of the function, which helps prevent
any memory leaks.

- Changes made to the mcxt.c file have been relocated to mcxtfuncs.c, which
now contains all the existing
 memory statistics-related functions along with the code for the proposed
function.

The overall flow of a request is as follows:

1. A client backend running the pg_get_process_memory_contexts function
creates a DSA and allocates memory
to store statistics, tracked by DSA pointer. This pointer is stored in a
DSHASH entry for each client querying the
statistics of any process.
The client shares its DSHASH table key with the server process using a
static shared array of keys indexed
by the server's procNumber.  It notifies the server process to publish
statistics by using SendProcSignal.

2. When a PostgreSQL server process handles the request for memory
statistics, the CFI function accesses the
client hash key stored in its procNumber slot of the shared keys array. The
server process then retrieves the
DSHASH entry to obtain the DSA pointer allocated by the client, for storing
the statistics.
 After storing the statistics, it notifies the client through its condition
variable.

3.  Although the DSA is created just once, the memory inside the DSA is
allocated and released by the client
process as soon as it finishes reading the statistics.
If it fails to do so, it is deleted by the before_shmem_exit callback when
the client exits. The client's entry in DSHASH
table is also deleted when the client exits.

4. The DSA and DSHASH table are not created
until  pg_get_process_memory_context function is called.
Once created, any client backend querying statistics and any PostgreSQL
process publishing statistics will
attach to the same area and table.

Please let me know your thoughts.

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>

---

From: Rahila Syed <[email-masked]>
Date: Tue, 29 Jul 2025 19:10:26 +0530

Hi,

Please find attached an updated patch. It contains the following changes.

1. It needed a rebase as highlighted by cfbot
<https://cfbot.cputube.org/patch_5938.log>.  The method for adding an
LWLock was updated in commit-2047ad068139f0b8c6da73d0b845ca9ba30fb33d, so
the patch has been adjusted to reflect this change.
2. Updated some comments to align with the latest patch design.
3. Eliminated an unnecessary assertion

Thank you,
Rahila Syed

On Fri, Jul 11, 2025 at 9:01 PM Rahila Syed <[email-masked]> wrote:

> This ensures the memory context created by the function does not affect
> its outcome.
> The memory context is reset at the end of the function, which helps
> prevent any memory leaks.
>
> - Changes made to the mcxt.c file have been relocated to mcxtfuncs.c,
> which now contains all the existing
> client hash key stored in its procNumber slot of the shared keys array.
> The server process then retrieves the
> DSHASH entry to obtain the DSA pointer allocated by the client, for
> storing the statistics.
>  After storing the statistics, it notifies the client through its
> condition variable.
>
> [2]. PostgreSQL: Re: Prevent an error on attaching/creating a DSM/DSA
> from an interrupt handler.

---

From: Rahila Syed <[email-masked]>
Date: Fri, 8 Aug 2025 14:56:52 +0530

Hi,

CFbot indicated that the patch requires a rebase, so I've attached an
updated version.
The documentation for this feature is now included in the new
func-admin.sgml file,
due to recent changes in the documentation of sql functions.

The following are results from a performance test:

pgbench is initialized as follows :
pgbench -i -s 100 postgres

Test1 -
pgbench -c 16 -j 16 postgres -T 100
TPS: 745.02 (average of 3 runs)

Test2-
pgbench -c 16 -j 16 postgres -T 100

while memory usage of any postgres process is monitored concurrently every
0.1 seconds,
using the following method:

SELECT * FROM pg_get_process_memory_contexts(
  (SELECT pid FROM pg_stat_activity
    ORDER BY random() LIMIT 1)
  , false, 5);

TPS: 750.66 (average of 3 runs)

I have not observed any performance decline resulting from the concurrent
execution
of the memory monitoring function.

Thank you,
Rahila Syed

On Tue, Jul 29, 2025 at 7:10 PM Rahila Syed <[email-masked]> wrote:

>> It has been redesigned to address several issues highlighted in the
>> thread [1]
>> and [2].
>>
>> -To address the memory leak concern, we create an exclusive memory
>> context under the NULL context, which
>> If it fails to do so, it is deleted by the before_shmem_exit callback
>> when the client exits. The client's entry in DSHASH

---

From: torikoshia <[email-masked]>
Date: Tue, 12 Aug 2025 09:34:00 +0900

On 2025-08-08 18:26, Rahila Syed wrote:
Hi, thanks for working on this again.

Here are some comments and questions for v32 patch:

> --- a/doc/src/sgml/func/func-admin.sgml
> +++ b/doc/src/sgml/func/func-admin.sgml
> @@ -251,6 +251,137 @@
>          <literal>false</literal> is returned.
>         </para></entry>
>        </row>
> +
> +      <row>
> +       <entry role="func_table_entry"><para role="func_signature">
> +        <indexterm>
> +         <primary>pg_get_process_memory_contexts</primary>

This function is added at the end of Table "9.96. Server Signaling 
Functions", but since pg_get_process_memory_contexts outputs essentially 
the same information as pg_log_backend_memory_contexts, it might be 
better to place them next to each other in the table.

> +     <parameter>stats_timestamp</parameter> <type>timestamptz</type> )

> +typedef struct MemoryStatsDSHashEntry
> +{
> +   char        key[64];
> +   ConditionVariable memcxt_cv;
> +   int         proc_id;
> +   int         total_stats;
> +   bool        summary;
> +   dsa_pointer memstats_dsa_pointer;
> +   TimestampTz stats_timestamp;
> +} MemoryStatsDSHashEntry;

stats_timestamp appears only in the two places below in the patch, but 
it does not seem to be actually output.
Is this column unnecessary?

   =# select * from pg_get_process_memory_contexts(pg_backend_pid(), 
true, 10);

   -[ RECORD 1 ]----+-----------------------------
   name             | TopMemoryContext
   ident            | [NULL]
   type             | AllocSet
   path             | {1}
   level            | 1
   total_bytes      | 222400
   total_nblocks    | 8
   free_bytes       | 4776
   free_chunks      | 8
   used_bytes       | 217624
   num_agg_contexts | 1

Specifying 0 for timeout causes a crash:

   =# select * from pg_get_process_memory_contexts(74526, true, 0);
   (0 rows)

   =# select 1;
   WARNING:  terminating connection because of crash of another server 
process
   DETAIL:  The postmaster has commanded this server process to roll back 
the current transaction and exit, because another server process exited 
abnormally and possibly corrupted shared memory.
   HINT:  In a moment you should be able to reconnect to the database and 
repeat your command.
   server closed the connection unexpectedly
           This probably means the server terminated abnormally
           before or while processing the request.

Should 0 be handled safely and treated as “no timeout”, or rejected as 
an error?

Similarly, specifying a negative value for timeout still works:

   =# select * from pg_get_process_memory_contexts(30590, true, -10);

It might be better to reject negative values similar to 
pg_terminate_backend().

>     context_id_lookup = 
> hash_create("pg_get_remote_backend_memory_contexts",

> + /* Retreive the client key fo publishing statistics */

fo -> for?

> + * times out, which is set to MEMSTATS_WAIT_TIMEOUT, retry given that 
> there is
> + * time left within the timeout specified by the user, before giving 
> up and
> + * returning previously published statistics, if any. If no previous 
> statistics
> + * exist, return NULL.
> + */
> +#define MEMSTATS_WAIT_TIMEOUT 100

MEMSTATS_WAIT_TIMEOUT is defined, but it doesn’t seem to be used.

-- 
Regards,

--
Atsushi Torikoshi
Seconded from NTT DATA Japan Corporation to SRA OSS K.K.

---

From: Rahila Syed <[email-masked]>
Date: Thu, 14 Aug 2025 04:05:59 +0530

Hi Torikoshia,

Thank you for reviewing the patch.

> better to place them next to each other in the table.
The idea was to place the new addition at the end of the table instead of
in the middle.
I’m fine with putting them together, though. I’ll do that in the next
version unless there’s a
reason not to.

> Is this column unnecessary?
Thank you for pointing this out. This is removed in the attached patch, as
it was a
remnant from the previous design.  As old statistics are discarded in the
new design,
a timestamp field is not needed anymore.

> Should 0 be handled safely and treated as “no timeout”, or rejected as

Good catch.
The crash has been resolved in the attached patch. It was caused by a
missing
ConditionVariableCancelSleep() call when exiting without statistics due to
a timeout value of 0.
A 0 timeout means that statistics should only be retrieved if they are
immediately available,
without waiting.  We could exit with a warning/error saying "too low
timeout", but I think it's worthwhile
to try fetching the statistics if possible.

> It might be better to reject negative values similar to
Fixed as suggested by you in the attached patch.
Currently, negative values are interpreted as an indefinite wait for
statistics.
This could cause the client to hang if the server process exits without
providing statistics.
To avoid this, it would be better to exit after displaying a warning when
the user specifies
negative timeouts.

Fixed.

> MEMSTATS_WAIT_TIMEOUT is defined, but it doesn’t seem to be used.
This is removed now as it was a leftover from the previous design.

The attached patch also fixes an assertion failure I observed when a client
times out
before the last requested process can publish its statistics. A client
frees the memory
reserved for storing the statistics when it exits the function after
timeout. Since a
server process was notified, it might attempt to read the same client entry
and access the dsa
memory reserved for statistics resulting in the assertion
failure.  I resolved this by including a check for this scenario and then
exiting the handler
function accordingly.

Thank you,
Rahila Syed

---

From: torikoshia <[email-masked]>
Date: Mon, 18 Aug 2025 22:32:09 +0900

On 2025-08-14 07:35, Rahila Syed wrote:
> The idea was to place the new addition at the end of the table instead
> of in the middle.
> I’m fine with putting them together, though. I’ll do that in the
> next version unless there’s a
> reason not to.
> 
> Thank you for pointing this out. This is removed in the attached
> patch, as it was a
> remnant from the previous design.  As old statistics are discarded in
> the new design,
> ConditionVariableCancelSleep() call when exiting without statistics
> due to a timeout value of 0.
> This could cause the client to hang if the server process exits
> without providing statistics.
> To avoid this, it would be better to exit after displaying a warning
> when the user specifies
> negative timeouts.
> 
> The attached patch also fixes an assertion failure I observed when a
> client times out
> server process was notified, it might attempt to read the same client
> entry and access the dsa
> failure.  I resolved this by including a check for this scenario and
> then exiting the handler
> function accordingly.

Thanks for updating the patch!
However, when I ran pg_get_process_memory_contexts() with summary = 
true, it took a while and returned nothing:

   =# select  pg_get_process_memory_contexts(pg_backend_pid(), true, 1) 
from pg_stat_activity ;

    pg_get_process_memory_contexts
   --------------------------------
   (0 rows)

   Time: 6026.291 ms (00:06.026)

Since v32 patch quickly returned the memory contexts as expected with 
the same parameter specified, there seems to be some degradation. Could 
you check it?

-- 
Regards,

---

From: Rahila Syed <[email-masked]>
Date: Wed, 20 Aug 2025 03:12:52 +0530

Hi,

>     pg_get_process_memory_contexts

Thank you for reporting this failure. This issue was a regression caused by
the absence of a
ConditionVariableSignal() call in the summary = true code path,
which happened due to recent code refactoring.

PFA the fix.

Thank you,
Rahila Syed

---

From: torikoshia <[email-masked]>
Date: Tue, 07 Oct 2025 09:51:15 +0900

Specifying a very small timeout value (such as 0 or 0.0001) and 
repeatedly executing the function seems to cause unexpected behavior. In 
some cases, it even leads to a crash.

For example:

   (session1)=# select pg_backend_pid();
    pg_backend_pid
   ----------------
             50917

   (session2)=# select pg_get_process_memory_contexts(50917, true,   
0.0001);
    pg_get_process_memory_contexts
   --------------------------------
   (0 rows)

   (session2)=# \watch 0.01

   (,,???,,0,0,0,0,0,0,0)
   ...
(21 rows)

   (session2)=# \watch 0.01

   ...

   server closed the connection unexpectedly
          This probably means the server terminated abnormally
          before or while processing the request.
   The connection to the server was lost. Attempting reset: Failed.

This issue occurs on my M1 Mac, but I couldn’t reproduce it on Ubuntu, 
so it might be environment-dependent.

Looking at the logs, Assert() is failing:

   2025-10-07 08:48:26.766 JST [local] psql [23626] WARNING:  01000: 
server process 23646 is processing previous request
   2025-10-07 08:48:26.766 JST [local] psql [23626] LOCATION:  
pg_get_process_memory_contexts, mcxtfuncs.c:476
   TRAP: failed Assert("victim->magic == FREE_PAGE_SPAN_LEADER_MAGIC"), 
File: "freepage.c", Line: 1379, PID: 23626
   0   postgres                            0x000000010357fdf4 
ExceptionalCondition + 216
   1   postgres                            0x00000001035cbe18 
FreePageManagerGetInternal + 684
   2   postgres                            0x00000001035cbb18 
FreePageManagerGet + 40
   3   postgres                            0x00000001035c84cc 
dsa_allocate_extended + 788
   4   postgres                            0x0000000103453af0 
pg_get_process_memory_contexts + 992
   5   postgres                            0x0000000103007e94 
ExecMakeFunctionResultSet + 616
   6   postgres                            0x00000001030506b8 
ExecProjectSRF + 304
   7   postgres                            0x0000000103050434 
ExecProjectSet + 268
   8   postgres                            0x0000000103003270 
ExecProcNodeFirst + 92
   9   postgres                            0x0000000102ffa398 
ExecProcNode + 60
   10  postgres                            0x0000000102ff5050 ExecutePlan 
+ 244
   11  postgres                            0x0000000102ff4ee0 
standard_ExecutorRun + 456
   12  postgres                            0x0000000102ff4d08 ExecutorRun 
+ 84
   13  postgres                            0x0000000103341c84 
PortalRunSelect + 296
   14  postgres                            0x0000000103341694 PortalRun + 
656
   15  postgres                            0x000000010333c4bc 
exec_simple_query + 1388
   16  postgres                            0x000000010333b5d0 
PostgresMain + 3252
   17  postgres                            0x0000000103332750 
BackendInitialize + 0
   18  postgres                            0x0000000103209e48 
postmaster_child_launch + 456
   19  postgres                            0x00000001032118c8 
BackendStartup + 304
   20  postgres                            0x000000010320f72c ServerLoop 
+ 372
   21  postgres                            0x000000010320e1e4 
PostmasterMain + 6448
   22  postgres                            0x0000000103094b0c main + 924
   23  dyld                                0x0000000199dc2b98 start + 
6076

Could you please check if you can reproduce this crash on your 
environment?

> <type>timestamptz</type> )

As discussed earlier, I believe we decided to remove stats_timestamp,
but it seems it’s still mentioned here.

> + * Update timestamp and signal all the waiting client backends after 
> copying
> + * all the statistics.
> + */
> +static void
> +end_memorycontext_reporting(MemoryStatsDSHashEntry *entry, 
> MemoryContext oldcontext, HTAB *context_id_lookup)

Should “Update timestamp” in this comment also be removed for 
consistency?

The column order differs slightly from pg_backend_memory_contexts.
If there’s no strong reason for the difference, perhaps aligning the 
order might improve consistency:

true, 1) ;
   name             | TopMemoryContext
   ident            | [NULL]
   type             | AllocSet
   path             | {1}
   level            | 1
   total_bytes      | 222400

   =# select * from pg_backend_memory_contexts;
   name          | TopMemoryContext
   ident         | [NULL]
   type          | AllocSet
   level         | 1
   path          | {1}
   total_bytes   | 99232
   ...

Regards,

---

From: Rahila Syed <[email-masked]>
Date: Thu, 9 Oct 2025 14:13:25 +0530

Hi Torikoshia,

Thank you for testing and reviewing the patch.

> Could you please check if you can reproduce this crash on your
I haven't been able to reproduce this issue on Ubuntu. A colleague also
tested it on their Mac
and didn't encounter the problem. I do have a fix in this area that I
believe should address an edge
case where data might be written to freed DSA memory.

Kindly test using the v35 patch and let me know if you still see the issue.

Fixed.

Fixed.

> order might improve consistency:
Makes sense. I will fix this in the next iteration of the patch.

I am also attaching a test which implements crash testing using injection
points.
I plan to improve the tests further to increase the test coverage of the
feature.

Thank you,
Rahila Syed

---

From: torikoshia <[email-masked]>
Date: Tue, 14 Oct 2025 18:00:45 +0900

On 2025-10-09 17:43, Rahila Syed wrote:
> I haven't been able to reproduce this issue on Ubuntu. A colleague
> also tested it on their Mac

Thanks for the update.

v35 works fine on my environment.
I ran the same test and haven’t encountered the crash anymore.
The addition of the following code appears to have resolved the issue:

   +memstats_dsa_cleanup(char *key)
   +{
   +   MemoryStatsDSHashEntry *entry;
   +
   +   entry = dshash_find(MemoryStatsDsHash, key, true);

Since you seem to make a next version patch, I understand v35 is an 
interim patch,
so this isn’t a major concern, but I encountered trailing whitespace 
warnings when applying the patches:

   $ git apply 
0001-v35-0001-Add-pg_get_process_memory_context-function.patch
   0001-v35-0001-Add-pg_get_process_memory_context-function.patch:705: 
trailing whitespace.
   0001-v35-0001-Add-pg_get_process_memory_context-function.patch:1066: 
trailing whitespace.

-- 
Regards,

---

From: Rahila Syed <[email-masked]>
Date: Tue, 28 Oct 2025 11:06:06 +0530

Hi,

PFA an updated v39 patch which is ready for review in the upcoming
commitfest.

> I ran the same test and haven’t encountered the crash anymore.

Thank you for testing and confirming the fix.

>    +   entry = dshash_find(MemoryStatsDsHash, key, true);

Yes, without this code, the dsa memory was being freed in the timeout path
without acquiring a lock.

> warnings when applying the patches.
>
   $ git apply
>    0001-v35-0001-Add-pg_get_process_memory_context-function.patch:1066:
Thanks, should be fixed now.

The updated patch contains the following changes. These changes are
addressing some review comments
discussed off list and a couple of bugs found while doing injection points
tests.

1.
All the changes made to MemoryContextStatsInternal and
MemoryContextStatsDetail are removed.
Instead of modifying these functions, I have written a separate function
MemoryContextStatsCounter
that takes care of counting statistics. This approach ensures that the
existing functions remain unchanged.

2. Changes to ensure that the wait loop does not exceed the prescribed wait
time.
Additional exit condition has been added to the infinite loop that waits
for request completion.
This allows the pg_get_memoy_context_statistics function to return if the
elapsed time goes beyond
a set limit i.e the following timeout.

3. The user facing timeout is removed as that would complicate the user
interface. CFIs
are called frequently and the requests are likely to be addressed promptly.
A predefined macro MEMORY_CONTEXT_STATS_TIMEOUT 5 (secs)  is used for
timeout
instead.  This would also remove the possibility of a user setting very low
timeouts, which
could cause requests to be incomplete and result in NULL outputs.

4. Miscellaneous cleanups to improve comments and remove left over comments
from older
versions. Also, removed an unnecessary argument from the
PublishMemoryContext function.

5. Addressed Torikoshias suggestion to change the order of columns to match
pg_backend_memory_contexts.

6. Attached is a test module that tests error handling by introducing
errors using
injection points. I have resolved a few bugs, so the memory monitoring
function
now runs correctly after the previous request ended with an error.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Sat, 8 Nov 2025 04:25:42 +0530

Hi,

I have attached a version 40 patch that has been rebased onto the
latest master branch, as CFbot indicated a rebase was needed.
The test module patch is unchanged.

Thank you,
Rahila Syed

On Tue, Oct 28, 2025 at 11:06 AM Rahila Syed <[email-masked]> wrote:

> 2. Changes to ensure that the wait loop does not exceed the prescribed
> wait time.
> instead.  This would also remove the possibility of a user setting very
> low timeouts, which
> 4. Miscellaneous cleanups to improve comments and remove left over
> comments from older

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 20 Nov 2025 10:56:43 +0100

> On 7 Nov 2025, at 23:55, Rahila Syed <[email-masked]> wrote:

> 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

---

From: Rahila Syed <[email-masked]>
Date: Tue, 25 Nov 2025 12:50:44 +0530

Hi Daniel,

Thank you for your comments. Please find attached v41 with all the comments
addressed.

> Are these actually required to be included?
Removed these.

> 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

> inline comments on each with their use?
Added comments.

> 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.

Fixed.

> in the variable definition can be removed per project style.
Fixed too.

> case the query errors out?
Good point. Added this just before the result set is getting populated.

> +       MemoryStatsDsHash =
> GetNamedDSHash("memory_context_statistics_dshash", &memctx_dsh_params,
> &found);
I have edited this accordingly.

> 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.

Fixed.

> 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_NUM
> MEMORY_CONTEXT_REPORT_MAX_PER_BACKEND / MAX_MEMORY_CONTEXT_STATS_SIZE
Fixed.

I've attached the test patch as is, I will clean it up and do further
improvements to it.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Fri, 28 Nov 2025 14:52:14 +0530

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-masked]> wrote:

> Hi Daniel,
>
> Thank you for your comments. Please find attached v41 with all the
> comments addressed.
>
>
>>

---

From: torikoshia <[email-masked]>
Date: Mon, 08 Dec 2025 10:26:33 +0900

On 2025-11-28 18:22, Rahila Syed wrote:

Hi,

> I'm attaching the updated patches, which primarily include cleanup and
> have been rebased

I observed an assertion failure when forcing a timeout as follows:

```
$ psql
   (pid:38587)=#

$ kill -s SIGSTOP 38587

$ psql
   (pid:38618) =# select * from pg_get_process_memory_contexts(38587, 
false);
     name  | ident  |  type  | level  |  path  | total_bytes | 
total_nblocks | free_bytes | free_chunks | used_bytes | num_agg_contexts
     
--------+--------+--------+--------+--------+-------------+---------------+------------+-------------+------------+------------------
      [NULL] | [NULL] | [NULL] | [NULL] | [NULL] |      [NULL] |        
[NULL] |     [NULL] |      [NULL] |     [NULL] |           [NULL]
     (1 row)

    Time: 5013.515 ms (00:05.014)

$ kill -s SIGCONT 38587

$ tail postgresql.log

   TRAP: failed Assert("client_keys[MyProcNumber] != -1"), File: 
"mcxtfuncs.c", Line: 881, PID: 38587
   0   postgres                            0x0000000104943400 
ExceptionalCondition + 216
   1   postgres                            0x000000010480f738 
ProcessGetMemoryContextInterrupt + 140
   2   postgres                            0x00000001046f2710 
ProcessInterrupts + 3008
   3   postgres                            0x00000001046f1a78 
ProcessClientReadInterrupt + 80
   4   postgres                            0x0000000104433994 secure_read 
+ 404
   5   postgres                            0x00000001044411dc pq_recvbuf 
+ 260
   6   postgres                            0x0000000104441088 pq_getbyte 
+ 96
   7   postgres                            0x00000001046fa0fc 
SocketBackend + 44
   8   postgres                            0x00000001046f6d3c ReadCommand 
+ 44
   9   postgres                            0x00000001046f6284 
PostgresMain + 2900
   10  postgres                            0x00000001046ed558 
BackendInitialize + 0
   11  postgres                            0x00000001045c0a48 
postmaster_child_launch + 456
   12  postgres                            0x00000001045c8520 
BackendStartup + 304
   13  postgres                            0x00000001045c636c ServerLoop 
+ 372
   14  postgres                            0x00000001045c4e24 
PostmasterMain + 6448
   15  postgres                            0x0000000104445b4c main + 924
   16  dyld                                0x0000000188662b98 start + 
6076
   2025-12-08 07:35:32.608 JST   [38540] LOG:  00000: client backend (PID 
38587) was terminated by signal 6: Abort trap: 6
   2025-12-08 07:35:32.608 JST   [38540] LOCATION:  LogChildExit, 
postmaster.c:2872
```

Below are comments regarding the v42-0001 patch:

> In order to not block on busy processes, we have hardcoded
> the number of seconds during which to retry before timing out.
> In the case where no statistics are published within the set
> timeout, NULL is returned.

It might be good to also document in func-admin.sgml that the function 
times out after 5 seconds when the target backend does not respond, and 
that in such a case NULLs are returned.

+    * If DSA exists, created by another process requesting statistics, 
attach
+    * to it. We expect the client process to create required DSA and 
Dshash
+    * table.
+    */
+   if (MemoryStatsDsaArea == NULL)
+       MemoryStatsDsaArea = 
GetNamedDSA("memory_context_statistics_dsa",
+                                        &found);
+
+   if (MemoryStatsDsHash == NULL)
+       MemoryStatsDsHash = 
GetNamedDSHash("memory_context_statistics_dshash",
+                                          &memctx_dsh_params, &found);

 From the comment, it sounded to me as if the client executing 
pg_get_process_memory_contexts() might not create the DSA in some cases.
Is it correct to assume that such a situation can happen?
In [1], as a response to concerns about using DSA inside a CFI handler, 
you wrote that “all the dynamic shared memory needed to store the 
statistics is created and deleted in the client function”.
So I understood that it would never create the DSA inside the CFI 
handler.
If that understanding is correct, perhaps the comment should be reworded 
to make that clear.

+   context_id_lookup = 

This appears to use the old function name. Should this be updated to 
"pg_get_process_memory_contexts" instead?

[1] 
https://www.postgresql.org/message-id/CAH2L28sc-rEhyntPLoaC2XUa0ZjS5ka6KzEbuSVxQBBnUYu1KQ%40mail.gmail.com

-- 
Regards,

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 8 Dec 2025 10:43:18 +0100

> On 28 Nov 2025, at 10:22, Rahila Syed <[email-masked]> wrote:
> 
> Hi,
> 
> I'm attaching the updated patches, which primarily include cleanup and have been rebased

Thanks for the patch, below are a few comments and suggestions.  As I was
reviewing I tweaked the below and have attached the comments as changes in
0003.

== in func-admin.sgml
+        <function>pg_get_process_memory_contexts</function> ( <parameter>pid</parameter> <type>integer</type>, <parameter>summary</parameter> <type>boolean</type> )
We recently simplified the UI by removing the timeout, and the more I think
about it the more I am convinced that there is more simplification to be had.
The most likely usage pattern, IMO, will be to get all the contexts and not the
summary, so we can make the summary parameter DEFAULT to false.  This allows
most uses to just pass the pid, without complicating the code at all.

== in mcxtfuncs.c
+/* Size of dshash key */
+#define CLIENT_KEY_SIZE 32
That's still pretty generous isn't it?  We are printing a uint32 into it so the
highest number it can reach is ~4 billion (which while the upper limit, is
quite theoretic in this case).  10 + 1 bytes should suffice to store right?

- * MemoryContextId
+ * MemoryStatsContextId
Sorry, but I still don't agree with this rename and I think we should skip it,
if only to avoid changes to existing parts of the code.

+    * Entry has been deleted due to client process exit. Make sure that the
+    * client always deletes the entry after taking required lock or this
+    * function may end up writing to unallocated memory.
Can you explain this a bit further, I'm not sure I get it.  The code goes on to
release a lock immediately and then destroys the hash.  Who is responsible for
destroying the entry?

== In system-views.sql
+REVOKE EXECUTE ON FUNCTION
+   pg_get_process_memory_contexts(integer, boolean) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION
+   pg_get_process_memory_contexts(integer, boolean) TO pg_read_all_stats;
This is not a view, and the functions aren't used to drive a view, so these
should not be defined here.  The above mentioned change to add DEFAULT handling
to the summary parameter fixes this in the attached.

== In ProcessGetMemoryContextInterrupt()
I'm not a fan of having to exit's from the function doing duplicative cleanups,
in the attached I've wrapped them in a conditional to just have one exit path.
What do you think about that?

== In PublishMemoryContext()
+   end_memorycontext_reporting(entry, oldcontext, context_id_lookup);
Looking at this more I don't really like that resetting the memory context is
done via a separate function, when that function must be called from the exact
right place to ensure CurrentMemoryContext is what it thinks it is.  It's all a
bit too magic.  Since this is only called in 3 places I would prefer to inline
the code in PublishMemoryContext().

== In PublishMemoryContext()
+   const char *ident = context->ident;
+   const char *name = context->name;
ident and name are defined as const, but they are later assigned to after the
initial assignment. I think we need to unconstify these.

== In PublishMemoryContext()
+       if (strlen(name) >= MEMORY_CONTEXT_NAME_SHMEM_SIZE)
We already have namelen which is set to exactly strlen(name), so let's reuse
that for readability.

== In PublishMemoryContext()
+   /* Allocate DSA memory for storing path information */
This comment is no longer accurate is it?  The DSA has already been allocated
at this point.

== In memutils.h
+#include "utils/dsa.h"
This is not needed.

I also did some smaller comment rewording and reflowing, some smaller cleanups
and a fresh pgindent/pgperltidy run. The attached 0003 contains the above.

--
Daniel Gustafsson

---

From: Rahila Syed <[email-masked]>
Date: Fri, 12 Dec 2025 15:16:46 +0530

Hi,

Sorry for the late response. Thank you for your reviewing and testing the
patch.

> I observed an assertion failure when forcing a timeout as follows:
Good catch. This assertion is no longer valid because of recent updates
that reset the client_keys slot for a request when the client exits with a
timeout.
To address this, I’ve replaced the assertion with a check for -1 and now
return
from the function in that case.

> that in such a case NULLs are returned.
Added this.

> If that understanding is correct, perhaps the comment should be reworded
Yes, your understanding is correct. I reworded the comment accordingly.

> "pg_get_process_memory_contexts" instead?
Modified this.

I will post the updated patch in response to Daniel's message that follows
your email.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Fri, 12 Dec 2025 16:34:37 +0530

Hi Daniel,

> reviewing I tweaked the below and have attached the comments as changes in

Thank you for the improvements.
All your changes look good to me. I have incorporated those in the v44
patch.

> release a lock immediately and then destroys the hash.  Who is responsible

This just points to the general requirements of taking a lock before
writing to a shared variable.
This serves as a warning to other processes not to delete the entry without
taking a lock, since
we are about to write to the entry.

> What do you think about that?

I agree with your approach.  It certainly makes the code more concise and
easier to read.

> This comment is no longer accurate is it?  The DSA has already been
Yes, it is not valid anymore. Fixed accordingly.

Apart from this, I cleaned up the test module by removing unnecessary sql
functions, added some more injection points based tests and a few
minor tweaks.

Please find attached updated and rebased patches.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Fri, 19 Dec 2025 01:24:42 +0530

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

---

From: Rahila Syed <[email-masked]>
Date: Mon, 29 Dec 2025 14:26:40 +0530

Hi,

I've included some additional description inline and attached rebased
patches after
CFbot reported a conflict.

> DSM into the current process address space and does not create a new DSM,
> I don't see any specific reason why
Following are the details about the use of DSM in the patch:

- DSA Creation: A Dynamic Shared Area (DSA) is used to store memory context
statistics.

- Client Process: When fetching memory context statistics, the client
allocates a 1 MB chunk
in the DSA,  reads the statistics from the memory chunk, copies it into a
tuple store, and then
frees the chunk.

- Storage: Pointers to these chunks are stored in a DSHASH table indexed by
the client’s
proc number.  Each entry in the DSHASH table also stores additional
metadata related to the
client’s request.

- Attachment: Backends only attach to the DSM segments for the DSA and
DSHASH table
when necessary i.e when a process queries memory context statistics or is
queried by
another backend.
Once attached, they remain so until the session ends, at which point they
remove their DSHASH
entry if any and detach from DSA and DSHASH segments.

- Lifecycle: The DSA and DSHASH structures are created upon the first SQL
function invocation
and destroyed on server restart.

> *Error Reported in Thread [2]*: This issue has been fixed by switching to
> a NULL resource owner before attaching
This error mentioned in thread [2] is triggered during CFI() call from
secure_read() when a
backend is waiting for commands and it has an open transaction which is
going to abort

Below are some details about this fix.

It is safe to temporarily set the resource owner to NULL before attaching
to the DSA
and DSHASH, since these segments are intended to be attached for the full
session
and are detached only when the session ends.
We also restore the original resource owner immediately after the attach
completes.

Other possible fixes include:
1.Adjusting resource‑owner behavior
Either allow resource‑owner enlargement during release, or delay marking it
as releasing until
the abort actually begins.
2. Updating DSM registry APIs (e.g., GetNamedDSA)
Detect when the current resource owner is in a releasing state and
temporarily set
CurrentResourceOwner to NULL before calling dsa_attach.
3. Handling it in the DSA layer
This was discussed in thread [2], but concerns were raised that DSA should
not compensate
for incorrect caller state; the caller must ensure the resource owner is
valid.

Kindly let me know your views.

Thank you,
Rahila Syed

an interrupt handler

---

From: Rahila Syed <[email-masked]>
Date: Fri, 9 Jan 2026 16:52:25 +0530

Hi,

>> *Error Reported in Thread [2]*: This issue has been fixed by switching
>> to a NULL resource owner before attaching
After further discussion and reviewing Robert's email[1] on this topic, a
safer solution
is to avoid running ProcessGetMemoryContextInterrupt during an aborted
transaction.
This should help prevent additional errors when the transaction is already
in error handling
state.  Also, reporting memory context statistics from an aborting
transaction won't
be very useful as some of that memory usage won't be valid after abort
completes.
Attached is the updated patch that addresses this.

> Either allow resource‑owner enlargement during release, or delay marking
> it as releasing until

Sorry, this point is invalid as resource-owner is already being marked as
releasing from
AbortTransaction.

Thank you,
Rahila Syed

[1].
https://www.postgresql.org/message-id/CA%2BTgmoapJ6erjT21uPO12wTtoOmj6w-dp6T3qySN%2BNSc1cdEKw%40mail.gmail.com

---

From: Robert Haas <[email-masked]>
Date: Tue, 13 Jan 2026 14:31:00 -0500

On Fri, Jan 9, 2026 at 6:22 AM Rahila Syed <[email-masked]> wrote:
> After further discussion and reviewing Robert's email[1] on this topic, a safer solution
> is to avoid running ProcessGetMemoryContextInterrupt during an aborted transaction.
> This should help prevent additional errors when the transaction is already in error handling
> state.  Also, reporting memory context statistics from an aborting transaction won't
> be very useful as some of that memory usage won't be valid after abort completes.

I think the question here is whether it's safe to interrupt the server
at an arbitrary CFI to run ProcessGetMemoryContextInterrupt. The good
news is that we now know that that function won't be executed (or at
least not in a substantive way) when we're an aborted transaction. But
we have no guarantee that we're in a transaction at all, which I think
is concerning, because it seems hard to write code that behaves
properly both inside of a transaction and outside of a transaction.
Some things I notice reading through:

+       LWLockAcquire(client_keys_lock, LW_SHARED);

LWLocks normally use NamesLikeThis not names_like_this and are
generally created by just listing them in lwlocklist.h.

Also, it's not really safe to acquire an LWLock if there's no
transaction active. If we error afterward, what will release the
LWLock?

+       else
+       {
+               clientProcNumber = client_keys[MyProcNumber];
+               client_keys[MyProcNumber] = -1;
+               LWLockRelease(client_keys_lock);
+       }

The "else" is not really necessary here because the "if" portion ends
with "return".

+       memstats_ctx = AllocSetContextCreate((MemoryContext) NULL,
+
          "publish_memory_context_statistics",
+
          ALLOCSET_SMALL_SIZES);

The comments do a good job justifying this, but as far as I know it
would be the only instance of this pattern in the entire source tree.
Are we really sure we want to deviate from the idea of having the
memory context tree be a tree? And is it really so bad if the memory
used to report memory contexts is included in the output?

+               MemoryStatsDsaArea =
GetNamedDSA("memory_context_statistics_dsa",
+
          &found);
...
+               MemoryStatsDsHash =
GetNamedDSHash("memory_context_statistics_dshash",
+
            &memctx_dsh_params, &found);
...
+       entry = dshash_find_or_insert(MemoryStatsDsHash, key, &found);

Like LWLockAcquire, all of this code supposes that there's a
transaction available to manage resource acquisition and release and
to clean up after errors. I doubt that any of this is safe without a
transaction.

I think my view overall here is: bailing out when the current
transaction is aborted is a good start in terms of making this safe,
but I don't think it's enough. If we get here when the backend is
idle, for example, which seems like it would be possible, then we'll
have no active transaction, and then I think that a lot of what's
being called here is not going to necessarily work as intended. There
are other places in the source code that do various push-ups to get
around this problem -- e.g. look at RemoveTempRelationsCallback, which
knows that it will be called from a transaction but not whether that
transaction will be aborted. I don't think that's the approach you
want here: restarting an aborted transaction in this context wouldn't
be smart. But spinning one up if there isn't one might be reasonable.
See ProcessCatchupInterrupt() for an example of existing code that
deals with this problem -- again that's probably not quite the same
situation, but it gives you an idea what other people have done
before.

One other point to note here is that if, by some chance, a failure
occurs as a result of receiving a memory-context interrupt, it's going
to fail the currently-running transaction. Unlike the problem
discussed in the previous paragraph, that's not a system-integrity
issue: from the point of view of PostgreSQL's transaction system,
having process #1 cause process #2's transaction to fail is 100% OK
and nothing will break. From the user's point of view, however, this
will be very painful: you just wanted to inspect the state of a
running transaction, very possibly a long-running one that had done a
lot of work, and you accidentally killed it. So I think it's going to
be important that such cases are extremely rare in practice. If they
happen because of extremely weird scenarios like somebody manually
removing a shared memory segment while it's still in use by
PostgreSQL, or the system running out of memory at exactly the wrong
moment, I think that's probably fine. If they happen because of
something like a low-probability race condition, that's going to be
unacceptable to users. Nobody is going to be happy about using the
feature if it can cause the transactions that they're querying to
randomly die, even if the chances are low. I'm not saying the patch
actually has this problem, just that it's something to think very
carefully about.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Andres Freund <[email-masked]>
Date: Tue, 13 Jan 2026 15:26:02 -0500

Hi,

> transaction active. If we error afterward, what will release the

All the error handling paths (hopefully) have an LWLockReleaseAll()... Which
is pretty crucial given that we do stuff outside of transactions in other
places.

That doesn't mean the other concerns about resource management are unfounded,
however.

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Tue, 13 Jan 2026 16:11:24 -0500

> That doesn't mean the other concerns about resource management are unfounded,

Yeah, I actually wasn't completely sure about that particular comment.
I think what will happen if we ERROR outside of a transaction is that
it will become FATAL and kill the backend, but I'm not 100% positive
about that.

---

From: Andres Freund <[email-masked]>
Date: Tue, 13 Jan 2026 16:47:41 -0500

Hi,

> it will become FATAL and kill the backend, but I'm not 100% positive

I'm pretty sure that doesn't generally happen. There's promotion to FATAL if
the top-level sigsetjmp() hasn't yet run (c.f. the check for
PG_exception_stack in errstart()), but once it has been reached, it stays
configured.

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Wed, 14 Jan 2026 08:26:24 -0500

> PG_exception_stack in errstart()), but once it has been reached, it stays

All right, then I guess I don't fully understand how the
error-outside-of-a-transaction case is handled. But I still think that
code like this needs to run in a transaction to avoid unexpected and
undesirable results. Do you see it differently?

---

From: Andres Freund <[email-masked]>
Date: Wed, 14 Jan 2026 08:32:36 -0500

Hi,

On 2026-01-14 08:26:24 -0500, Robert Haas wrote:
> error-outside-of-a-transaction case is handled.

For those we just rely on the error cleanup inside the top-level sigsetmp()
blocks... Which of course is a pretty random subset of cleanups that also
differs between process types, which is quite ... not great.

> But I still think that

I'm waffling on it a bit, tbh. I think doing it for something like backends it
could make sense, but e.g. for something like checkpointer, that normally
never runs a transaction, it seems like a bad idea. Mainly because processes
that don't run transactions won't have an AbortCurrentTransaction() or such in
their top-level sigsetjmp() and thus would never abort the transaction that we
started, if there were an error while doing the reporting.

Greetings,

Andres Freund

---

From: Rahila Syed <[email-masked]>
Date: Mon, 19 Jan 2026 23:01:17 +0530

Hi,

Thank you for your feedback. Please find a few responses inline.

I am working on incorporating all the feedback in the patch and will share
it in a follow-up email.

> generally created by just listing them in lwlocklist.h.
I will fix it, accordingly.

> The "else" is not really necessary here because the "if" portion ends

Fixed this in the attached patch.

> used to report memory contexts is included in the output?
This was implemented in response to a review suggestion [1].
If required, it can be updated to create one under CurrentMemoryContext.

+               MemoryStatsDsaArea =
> to clean up after errors. I doubt that any of this is safe without a

Starting a transaction from CFI works for a client backend but
not for auxiliary processes. When I try to execute StartTransaction()
from a checkpointer process, the following assertion fails,
Assert(MyProc->vxid.procNumber == vxid.procNumber);
This is because MyProc->vxid.procNumber is not set for auxiliary
processes.

Please find attached a rebased patch, that fixes a build error reported
on the commit-fest app.

[1]. PostgreSQL: Re: Prevent an error on attaching/creating a DSM/DSA from
an interrupt handler.
<https://www.postgresql.org/message-id/594293.1747708165%40sss.pgh.pa.us>

Thank you,
Rahila Syed

---

From: Robert Haas <[email-masked]>
Date: Mon, 19 Jan 2026 13:02:27 -0500

> This is because MyProc->vxid.procNumber is not set for auxiliary

Unfortunately, somebody is going to need to think through - and
perhaps test - what happens in each individual type of background
process -- not just auxiliary processes but also background workers,
including but not limited to parallel workers. Some of them have error
recovery logic that is similar to transaction cleanup but only covers
a subset of things, in which case the question will be whether that
logic handles all the kinds of resources that this code might acquire.
Some of them may just straight up kill the process if an error occurs,
which is fine for this patch as long as it only happens in extreme
situations (e.g. OOM).

In other words, we don't necessarily need a transaction specifically,
but we need whatever form of error recovery is in use in any given
process to be appropriate to the code that this patch proposes to
execute. For a regular backend, that's a transaction.

Also worth noting: I don't think that all backend types actually use
CHECK_FOR_INTERRUPTS(). For those that don't, other updates may be
needed to make this feature work.

---

From: Rahila Syed <[email-masked]>
Date: Mon, 2 Feb 2026 17:58:21 +0530

Hi Robert, Daniel

Please find attached the updated patches which incorporate your feedback.

Following are responses to Robert's comments:

> which is fine for this patch as long as it only happens in extreme

I tested the resource cleanup in case of error, with respect to each of the
processes types i.e Auxiliary process/background workers/client backends
and below are my findings.

The only resources acquired within the ProcessGetMemoryContextInterrupt
function are of the kind dsm_resowner_desc through GetNamedDSA and
GetNamedDsHash.
However, these functions make the resource owner forget these resources
when they call dsm_pin_mapping. This is done to prevent them from
getting detached at the time of error handling or transaction abort/commit.
Because when a dsm resource is released, the associated dsm segment is
detached.

Since we want the DSM segments to remain mapped until a process
exits, they are not added to resource owners. This is done intentionally to
prevent repeatedly attaching or detaching the DSM segments, if the same
process is queried for statistics multiple times.

As no resources are tracked during the execution
ProcessGetMemoryContextInterrupt
irrespective of its execution within or outside a transaction, the error
handling
in both the cases does not process resource cleanup unless the process
exits.
During proc_exit, the cleanup/detach runs via dsm_backend_shutdown().

Fixed.

> transaction active. If we error afterward, what will release the

I verified that when an error occurs outside of a transaction in various
types of processes, LWLockReleaseAll() is called during error handling.
1. For auxiliary processes - called within sigsetjmp() from main functions.
2. For client backend - called during process exit as error is converted to
"FATAL:  terminating connection because protocol synchronization was lost"
3. For a background worker like parallel worker - called during process
exit,
as the worker is killed on encountering an error.

+       else
> with "return

Fixed accordingly.

to fail the currently-running transaction

In the updated version, we make sure that no errors are being thrown
by ProcessMemoryContextInterrupt and other functions added by this
patch.
I have also added comments above the GetNamedDSA and GetNamedDsHash
functions to highlight the importance of minimizing errors in these
functions,
as they are called from CFI.
This way we try to prevent any non-critical errors from being thrown.

Following are responses to Daniel's comments:

> A few small comments on v47:
>
> +static const char *ContextTypeToString(NodeTag type);
> I think context_type_to_string() would be a better name on this internal
> function to model it closer to the existing int_list_to_array().
> Personally I
> would also place it before its first use to avoid the prototype, but that's
> personal preference.
>
>
Changed accordingly, also moved compute_context_path() so it appears
before its use to avoid the prototype.

> This function warrants a documentation comment describing when it should be
> used safely.
>

 Added a comment.

>
>
> +           memstats_dsa_cleanup(key);
> +           memstats_client_key_reset(procNumber);
> +           ConditionVariableCancelSleep();
> +           PG_RETURN_NULL();
> I think we should notify the user in these two timeout cases, why not
> adding an
> ereport(NOTICE, "request for memory context statistics timed out")); or
> something with a better wording than that.
>
>  Added this.

> +   Size        sz = 0;
> +   Size        TotalProcs = 0;
> +
> +   TotalProcs = add_size(TotalProcs, NUM_AUXILIARY_PROCS);
> +   TotalProcs = add_size(TotalProcs, MaxBackends);
> +   sz = add_size(sz, mul_size(TotalProcs, sizeof(int)));
> +
> +   return sz
> As we discussed off-list, the call to add_size() call can be omitted as it
> won't affect the calculation.
>

Fixed accordingly.

>
>
> +# Copyright (c) 2025, PostgreSQL Global Development Group
> Here, and possibly elsewhere, it should say 2026 instead I think.
>
>
> Fixed.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Tue, 10 Feb 2026 05:33:37 +0530

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

---

From: Chao Li <[email-masked]>
Date: Tue, 10 Feb 2026 12:50:53 +0800

> On Feb 10, 2026, at 08:03, Rahila Syed <[email-masked]> wrote:
> 
> Hi,
> 
> <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/

---

From: Rahila Syed <[email-masked]>
Date: Sun, 15 Feb 2026 18:10:06 +0530

Hi Chao,

Thank you for testing and reviewing. Please find attached the updated
patches
that incorporate your review comments.

Added this.

> *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.
>

Fixed accordingly.

>
> +       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?
>
>
Makes sense, done in the attached patch.

> 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.
>

I  have incorporated your suggestion in the updated patch—using integer
keys will be more efficient
for memory usage and will help avoid conversions between data types.
Thank you for the suggestion.

+static List *
> 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?
>

We need to calculate the full path list to report the number of levels in
the tree.
We could choose to return the number of levels separately from the path
list and limit the
path list to no more than 100 levels, but we would still have to iterate
through all the levels.
Returning a single variable that captures both levels and path keeps the
function signature simple.
Kindly let me know your view.

> 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.
>
>
Good catch. Fixed accordingly.

>      (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.
>
The following check in the beginning of the wait loop is intended to avoid
this.

      /* Return if we have already exceeded the timeout */
        if (elapsed_time >= MEMORY_STATS_MAX_TIMEOUT * 1000)
        {
            memstats_dsa_cleanup(client_procno);
            memstats_client_key_reset(server_procno);
            ConditionVariableCancelSleep();
            ereport(NOTICE,
                    errmsg("request for memory context statistics for PID
%d timed out",
                           pid));
            PG_RETURN_NULL();
        }

It may still exceed by an amount less than 5 seconds, but it will always be
less
than  2 * MEMORY_STATS_MAX_TIMEOUT, because
we track the elapsed time and exit after the condition above is satisfied.
Maybe we should edit the docs to reflect this.

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.
>
>
The client backend deletes the entry only upon exiting; no other process
can remove the entry.
Therefore, it is not possible for the client backend to execute this Assert
after the entry has been
deleted.

>
> 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.
>
>
Fixed. Thank you for noticing this. It looks like a remnant from earlier
versions.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Sat, 21 Feb 2026 04:43:17 +0530

Hi,

Please find attached updated and rebased patches which incorporate
code fixes suggested off-list by Daniel

These changes include a fix to free dsa memory in one of the
exit paths from the client-side function along with several documentation
and comment improvements.

> into ConditionVariableTimedSleep.
This comment by Chao Li was also addressed as part of Daniel's review
fixes.

Thank you,
Rahila Syed

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 23 Feb 2026 10:58:39 +0100

> On 21 Feb 2026, at 00:13, Rahila Syed <[email-masked]> wrote:

> and comment improvements.

Thanks for the new version, I think this is looking pretty good now.  The
attached .diff.txt (renamed to keep the CFBot from grabbing it) has mostly
whitespace fixes, but also a few small things which are discussed below:

+       <para>
+        After receiving memory context statistics from the target process, it
+        returns the results as one row per context.  If all the contexts don't

Re-reading the docs I think the para's are in the wrong order, as the sentence
above makes little sense to be after the output has already been discussed.  I
think this needs to go earlier, see the attached .diff.txt for what I am
proposing.

+   /*
+    * The client process should have created the required DSA and DSHash
+    * table. Here we just attach to those.
+    */
+   if (MemoryStatsDsaArea == NULL)
+       MemoryStatsDsaArea = GetNamedDSA("memory_context_statistics_dsa",
+                                        &found);
+
+   if (MemoryStatsDsHash == NULL)
+       MemoryStatsDsHash = GetNamedDSHash("memory_context_statistics_dshash",
+                                          &memctx_dsh_params, &found);

I think this needs an expanded comment discussing why there is no need the
check the DSA and DShash after the GetNamed_ functions, since they can be NULL
going in to this.  The attached .diff.txt has a proposal along with an
Assertion to keep future changes of the API from risking subtly breaking this
assumption.

+REGRESS = test_memcontext_reporting

Since the test module doesn't contain any pg_regress style .sql/.out tests so
this line in the Makefile cause a test failure when running with Autoconf.

+$node->append_conf(
+   'postgresql.conf',
+   qq[
+max_connections = 100
+log_statement = none
+restart_after_crash = false
+]);

Why do we need to set max_connections for this test?

+#Server should have thrown error
+$node->psql(
+   'postgres',
+   qq(select pg_get_process_memory_contexts($pid, true);),
+   stderr => \$psql_err);

This test doesn't validate that the server actually errored does it?  (There is
no proposed fix in the attached.)

--
Daniel Gustafsson

diff --git a/doc/src/sgml/func/func-admin.sgml b/doc/src/sgml/func/func-admin.sgml
index 120f032f249..aa83b11e131 100644
--- a/doc/src/sgml/func/func-admin.sgml
+++ b/doc/src/sgml/func/func-admin.sgml
@@ -274,10 +274,19 @@
        <para>
         This function handles requests to display the memory contexts of a
         <productname>PostgreSQL</productname> process with the specified
-        process ID.  The function can be used to send requests to backends as
-        well as <glossterm linkend="glossary-auxiliary-proc">auxiliary processes</glossterm>.
-        If the process does not respond with memory contexts statistics in 5 seconds,
-        the function returns NULL.
+        process ID.  The function can be used to send requests to
+        <glossterm linkend="glossary-backend">backends</glossterm> as well as
+        <glossterm linkend="glossary-auxiliary-proc">auxiliary processes</glossterm>.
+        If the process does not respond with memory contexts statistics in 5
+        seconds, the function returns NULL.
+       </para>
+       <para>
+        After receiving memory context statistics from the target process, it
+        returns the results as one row per context.  If all the contexts don't
+        fit within the pre-determined size limit, the remaining context
+        statistics are aggregated and a cumulative total is displayed.  The
+        <literal>num_agg_contexts</literal> column indicates the number of
+        contexts aggregated in the displayed statistics.
        </para>
        <para>
         The returned record contains extended statistics per each memory
@@ -350,7 +359,8 @@
          <listitem>
           <para>
            <parameter>num_agg_contexts</parameter> - The number of memory
-           contexts aggregated in the displayed statistics.
+           contexts aggregated in the displayed statistics.  <literal>1</literal>
+           indicates that context statistics are displayed separately.
           </para>
          </listitem>
         </itemizedlist>
@@ -365,16 +375,6 @@
         <parameter>summary</parameter> is <literal>false</literal> (the default),
         <literal>the num_agg_contexts</literal> value is <literal>1</literal>,
         indicating that individual statistics are being displayed.
-       </para>
-       <para>
-        After receiving memory context statistics from the target process, it
-        returns the results as one row per context.  If all the contexts don't
-        fit within the pre-determined size limit, the remaining context
-        statistics are aggregated and a cumulative total is displayed.  The
-        <literal>num_agg_contexts</literal> column indicates the number of
-        contexts aggregated in the displayed statistics.  A
-        <literal>num_agg_contexts</literal> value of  <literal>1</literal>
-        indicates that context statistics are displayed separately.
        </para></entry>
       </row>
      </tbody>
diff --git a/src/backend/utils/adt/mcxtfuncs.c b/src/backend/utils/adt/mcxtfuncs.c
index f18b6d47c60..fdf0043211b 100644
--- a/src/backend/utils/adt/mcxtfuncs.c
+++ b/src/backend/utils/adt/mcxtfuncs.c
@@ -978,7 +978,11 @@ ProcessGetMemoryContextInterrupt(void)
 
 	/*
 	 * The client process should have created the required DSA and DSHash
-	 * table. Here we just attach to those.
+	 * table, so we can connect to these directly.  The GetNamedXXX functions
+	 * either return DSA/DSHash or error out (which we want to avoid as that
+	 * would cause the current transaction to fail) so we don't need to check
+	 * for NULL.  An assertion is enough to catch in case this API should be
+	 * changed at some point.
 	 */
 	if (MemoryStatsDsaArea == NULL)
 		MemoryStatsDsaArea = GetNamedDSA("memory_context_statistics_dsa",
@@ -988,6 +992,8 @@ ProcessGetMemoryContextInterrupt(void)
 		MemoryStatsDsHash = GetNamedDSHash("memory_context_statistics_dshash",
 										   &memctx_dsh_params, &found);
 
+	Assert(MemoryStatsDsaArea != NULL && MemoryStatsDsHash != NULL);
+
 	/*
 	 * The entry lock is held by dshash_find_or_insert to protect writes to
 	 * process specific memory. Two different processes publishing statistics
@@ -1001,9 +1007,8 @@ ProcessGetMemoryContextInterrupt(void)
 	 * if the caller has timed out waiting for us and have issued a request to
 	 * another backend.
 	 *
-	 * Make sure that the client always deletes the entry after taking
-	 * required lock or this function may end up writing to unallocated
-	 * memory.
+	 * Make sure that the client always deletes the entry after taking the
+	 * required lock or this function may end up writing to unallocated memory.
 	 */
 	if (!found || entry->target_server_id != MyProcPid)
 	{
@@ -1042,13 +1047,11 @@ ProcessGetMemoryContextInterrupt(void)
 														  HASH_ENTER, &found);
 		Assert(!found);
 
-		/*
-		 * context id starts with 1
-		 */
+		/* context id starts with 1 */
 		contextid_entry->context_id = cxt_id + 1;
 
 		/*
-		 * Copy statistics for each of TopMemoryContexts children.  This
+		 * Copy statistics for each of TopMemoryContext's children.  This
 		 * includes statistics of at most 100 children per node, with each
 		 * child node limited to a depth of 100 in its subtree.
 		 */
diff --git a/src/test/modules/test_memcontext_reporting/Makefile b/src/test/modules/test_memcontext_reporting/Makefile
index 47bce45bc1d..2ce169ffa69 100644
--- a/src/test/modules/test_memcontext_reporting/Makefile
+++ b/src/test/modules/test_memcontext_reporting/Makefile
@@ -11,8 +11,6 @@ OBJS = \
    test_memcontext_reporting.o
 PGFILEDESC = "test_memcontext_reporting - test code for memory context reporting"
 
-REGRESS = test_memcontext_reporting
-
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl b/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
index 73665abf902..152465c1570 100644
--- a/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
+++ b/src/test/modules/test_memcontext_reporting/t/001_memcontext_inj.pl
@@ -21,7 +21,6 @@ $node->init;
 $node->append_conf(
 	'postgresql.conf',
 	qq[
-max_connections = 100
 log_statement = none
 restart_after_crash = false
 ]);
@@ -36,7 +35,7 @@ $node->safe_psql('postgres',
 my $pid = $node->safe_psql('postgres',
 	"SELECT pid from pg_stat_activity where backend_type='checkpointer'");
 
-#Client should have thrown error
+# Client should have thrown error
 $node->psql(
 	'postgres',
 	qq(select pg_get_process_memory_contexts($pid, true);),
@@ -44,7 +43,8 @@ $node->psql(
 like($psql_err,
 	qr/error triggered for injection point memcontext-client-injection/);
 
-#Query the same process after detaching the injection point, using some other client and it should succeed.
+# Query the same process after detaching the injection point, using some other
+# client and it should succeed.
 $node->safe_psql('postgres',
 	"select injection_points_detach('memcontext-client-injection');");
 my $topcontext_name = $node->safe_psql('postgres',
@@ -57,13 +57,14 @@ $node->safe_psql('postgres',
 	"select injection_points_attach('memcontext-server-injection', 'error');"
 );
 
-#Server should have thrown error
+# Server should have thrown error
 $node->psql(
 	'postgres',
 	qq(select pg_get_process_memory_contexts($pid, true);),
 	stderr => \$psql_err);
 
-#Query the same process after detaching the injection point, using some other client and it should succeed.
+# Query the same process after detaching the injection point, using some other
+# client and it should succeed.
 $node->safe_psql('postgres',
 	"select injection_points_detach('memcontext-server-injection');");
 $topcontext_name = $node->safe_psql('postgres',
@@ -71,8 +72,8 @@ $topcontext_name = $node->safe_psql('postgres',
 );
 ok($topcontext_name eq 'TopMemoryContext');
 
-# Test that two concurrent requests to the same process results in a warning for
-# one of those
+# Test that two concurrent requests to the same process results in a warning
+# for one of those
 
 $node->safe_psql('postgres',
 	"SELECT injection_points_attach('memcontext-client-injection', 'wait');");
@@ -80,8 +81,9 @@ $node->safe_psql('postgres',
 	"SELECT injection_points_attach('memcontext-server-wait', 'wait');");
 my $psql_session1 = $node->background_psql('postgres');
 
-# We use query_until so that we can execute additional commands without having to
-# wait for this command to complete, since it will pause at injection points.
+# We use query_until so that we can execute additional commands without having
+# to wait for this command to complete, since it will pause at injection
+# points.
 
 $psql_session1->query_until(
 	qr//,
@@ -95,7 +97,7 @@ $node->psql(
 	stderr => \$psql_err);
 ok($psql_err =~
 	  /WARNING:  server process $pid is processing previous request/);
-#Wake the client up.
+# Wake the client up.
 $node->safe_psql('postgres',
 	"SELECT injection_points_wakeup('memcontext-client-injection');");
 
@@ -104,7 +106,8 @@ $node->safe_psql('postgres',
 $node->safe_psql('postgres',
 	"select injection_points_detach('memcontext-server-wait');");
 
-# Test the client process exiting with timeout does not break the server process
+# Test the client process exiting with timeout does not break the server
+# process
 
 $node->safe_psql('postgres',
 	"SELECT injection_points_attach('memcontext-server-wait', 'wait');");
@@ -114,12 +117,12 @@ $node->psql(
 	qq(select name from pg_get_process_memory_contexts($pid, true) where path = '{1}';),
 	stdout => \$psql_out);
 is($psql_out, '', 'Query returns null');
-#Wakeup the server process up and detach the injection point.
+# Wakeup the server process up and detach the injection point.
 $node->safe_psql('postgres',
 	"SELECT injection_points_wakeup('memcontext-server-wait');");
 $node->safe_psql('postgres',
 	"select injection_points_detach('memcontext-server-wait');");
-#Query the same server process again and it should succeed.
+# Query the same server process again and it should succeed.
 $topcontext_name = $node->safe_psql('postgres',
 	"select name from pg_get_process_memory_contexts($pid, true) where path = '{1}';"
 );
@@ -130,7 +133,7 @@ $node->safe_psql('postgres',
 	"select injection_points_attach('memcontext-client-injection', 'test_memcontext_reporting', 'crash', NULL);"
 );
 
-#Client will crash
+# Client will crash
 $node->psql(
 	'postgres',
 	qq(select name from pg_get_process_memory_contexts($pid, true) where path = '{1}';),
@@ -143,7 +146,7 @@ like($psql_err,
 $node->restart;
 $node->poll_query_until('postgres', "SELECT 1;", '1');
 
-#Querying memory stats should succeed after server start
+# Querying memory stats should succeed after server start
 $pid = $node->safe_psql('postgres',
 	"SELECT pid from pg_stat_activity where backend_type='checkpointer'");
 $topcontext_name = $node->safe_psql('postgres',

---

From: Rahila Syed <[email-masked]>
Date: Tue, 24 Feb 2026 17:27:17 +0530

Hi Daniel,

Thank you for the review. All the changes suggested in the v52comments.diff
are incorporated in the attached patches.

> no proposed fix in the attached.)
This has been fixed by adding a check for the error returned by the above
command.
While at it, I also added another crash test to the file,  This is similar
to the existing
test for a client backend crash, but in this scenario, it crashes the
server process
instead.

Thank you,
Rahila Syed

---

From: Rahila Syed <[email-masked]>
Date: Fri, 6 Mar 2026 04:01:09 +0530

Hi All,

The email below provides a summary of the changes implemented since the
previous
update on December 18th.

1. I tested and verified that when ProcessGetMemoryContextInterrupt()
returns with error
from processes that don't start a transaction, it doesn't affect system
integrity.
In auxiliary processes like ioworker, archiver, and wal_receiver, the ERROR
causes
the process to exit, which ensures proper resource and lock release. In
other cases,
cleanup and release are managed by error recovery logic, allowing processes
to continue
running smoothly.

2. I verified that the only resources acquired in
ProcessGetMemoryContextInterrupt
are DSMs, which remain attached until the process exits.
During proc_exit, the release/detach of DSM runs via dsm_backend_shutdown().

3.The error reporting level in the interrupt handler has been adjusted from
ERROR
to NOTICE to prevent unnecessary errors from being raised in interrupted
transactions
due to minor issues.

4. Aborted transaction check has been added to allow returning without
processing
the interrupt if it is called from an aborted transaction.

5. DSHash key type has been changed from string to integer, now using
procNumber
directly. This eliminates string formatting overhead.

6. Timeout messaging has been changed from silently returning empty row to
issuing
a NOTICE and returning an empty row.

7. Documentation has been to GetNamedDSA() and GetNamedDSHash(), warning
that
errors thrown from these functions can propagate to any transaction calling
the CFI function

8. Subtracted elapsed time ((MEMORY_STATS_MAX_TIMEOUT * 1000) -
elapsed_time) from
timeout passed to ConditionVariableTimedSleep so the total wait doesn't
exceed the intended
timeout in case of spurious wakeups.

9. Fixed a race condition bug by ensuring ConditionVariableSignal is called
before
dshash_release_lock

Additionally, I included couple of assertions, renamed variables and
functions to enhance
clarity and consistency with the existing style, and made improvements to
the documentation
and tests.

Thank you,
Rahila Syed