Subject: Re: pgsql: Add function to get memory context stats for processes

From: Robert Haas <[email-masked]>
Date: Mon, 14 Apr 2025 10:03:28 -0400

On Thu, Apr 10, 2025 at 4:05 PM Andres Freund <[email-masked]> wrote:
> I don't know of existing discussion, but it seems rather fundamental to me -
> if either DSA or memory contexts could be inconsistent at a CFI(), how could
> it possibly be safe to interrupt at that point? After all, after an error you
> need to be able to reset the memory contexts / release memory in a
> dsa/dshash/whatnot? Memory context reset requires walking over the allocations
> made in the context, similar releasing a dsa?

I think it would be a bit surprising if somebody put a
CHECK_FOR_INTERRUPTS() inside aset.c or similar, but I don't see a
reason why we couldn't end up with one reachable via the DSA code. DSA
calls DSM which depending on dynamic_shared_memory_type might involve
filesystem operations. That's a fairly large amount of code. I admit I
have no particular theory about how CFI could be reachable from there
today, but even if it definitely isn't, I don't see why someone would
hesitate to add one in the future.

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

---

From: Andres Freund <[email-masked]>
Date: Mon, 14 Apr 2025 10:13:27 -0400

Hi,

> hesitate to add one in the future.

There very well could be a CFI - but it better be somewhere where the
in-memory state is consistent. Otherwise an error inside raised in the CFI
would lead the in-memory state inconsistent which then would cause problems
when cleaning up the dsa during resowner release or process exit.

What am I missing here?

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Thu, 17 Apr 2025 10:42:45 -0400

> when cleaning up the dsa during resowner release or process exit.

I think maybe you're only thinking about gathering the data. What
about publishing it? If the DSA code were interrupted at a CFI and the
interrupting code went and tried to perform a DSA allocation to store
the resulting data and then returned to the interrupted DSA operation,
would you expect the code to cope with that? I do not believe we have
anywhere enough guarantees about reentrancy for that to be safe.

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 22 Apr 2025 10:11:43 +0200

> On 17 Apr 2025, at 16:42, Robert Haas <[email-masked]> wrote:
> 

Do you mean that an interrupt handler will make DSA allocations?  I don't think
that would be something we'd want to allow regardless of this patch.  Or did I
misunderstand the above?

--
Daniel Gustafsson

---

From: Andres Freund <[email-masked]>
Date: Tue, 22 Apr 2025 12:58:28 -0400

Hi,

On 2025-04-17 10:42:45 -0400, Robert Haas wrote:
> would you expect the code to cope with that?

I would expect the DSA code to not call CFI() in such a place - afaict nearly
all such cases would also not be safe against errors being raised in the CFI()
and then later again allocating memory from the DSA (or resetting
it). Similarly, aset.c better not accept interrupts in the middle of, e.g.,
AllocSetAllocFromNewBlock(), since we'd loose track of the allocation.

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Wed, 23 Apr 2025 12:12:11 -0400

> that would be something we'd want to allow regardless of this patch.  Or did I

My primary concern about the patch is that
ProcessGetMemoryContextInterrupt() can be called from any
CHECK_FOR_INTERRUPTS() and calls lots of DSA functions, including
dsa_create() and, via PublishMemoryContext(), dsa_allocate0(). I'm
shocked to hear that you and Andres think that's safe to do at any
current or future CHECK_FOR_INTERRUPTS() anywhere in the code; but
Andres seems very confident that it's fine, so perhaps I should just
stop worrying and be happy that we have the feature.

I thought that the issues here would be similar to
https://commitfest.postgresql.org/patch/5473/ and
https://commitfest.postgresql.org/patch/5330/ where it seems we need
to go to fairly extraordinary lengths to try to make the operation
safe -- the proposal there is essentially to have a CFI handler run
around the executor state tree and replace every ExecProcNode pointer
with a pointer to some new wrapper function which in turn does the
actual query-plan printing. Even that requires some tricky footwork to
get right, but the core idea is that you can't imagine that
CHECK_FOR_INTERRUPTS() is a safe place to do arbitrary stuff, so you
have to use it to trigger the actual work at some later point where it
will be safe to do the stuff that you want to do. I suppose the
difference in this case is that memory-allocation is a lower-level
subsystem than query execution and therefore there are presumably
fewer places where it's not safe to assume that you can do
memory-allocation type operations. But I at least feel like DSA is a
pretty high-level system that I wouldn't want to count on being able
to access from a fairly-arbitrary point in the code, which is why I'm
quite astonished to hear Andres basically saying "don't worry, it's
all fine!". But my astonishment does not mean that he is wrong.