On 23/11/2025 16:45, Joel Jacobson wrote:
> Hi hackers,
>
> This patch addresses this comment in async.c's SignalBackends:
>
> * XXX in principle these pallocs could fail, which would be bad.
> * Maybe preallocate the arrays? They're not that large, though.
>
> This is unsafe, since AtCommit_Notify effectively runs in a critical
> section, so an OOM there would PANIC ("AbortTransaction while in COMMIT
> state"), as we can no longer abort safely.
Ugh. I wonder if we should put an actual critical section around those
post-commit cleanup actions? As you said, it's effectively a critical
section already, except that we don't get the benefit of the
AssertNotInCriticalSection assertions.
Or even better, could we move things out of that effective critical
section? It's painful to write code that cannot palloc.
> This patch fixes this by adding two static arrays, notifySignalPids and
> notifySignalProcs, allocated lazily in TopMemoryContext by
> initSignalArrays. PreCommit_Notify now calls initSignalArrays while it's
> still safe to ERROR, ensuring the arrays exist before entering the
> commit path.
>
> SignalBackends is updated to use these preallocated arrays instead of
> allocating temporary ones.
>
> This removes the risk of palloc in a critical section and eliminates
> repeated palloc/pfree cycles.
Makes sense as far as it goes. But there's more: Exec_ListenCommit()
also palloc's, And AtCommit_Notify also calls asyncQueueAdvanceTail(),
which calls SimpleLruTruncate(). I didn't analyze SimpleLruTruncate() in
detail, but I bet it also palloc's, and it's not nice to panic e.g.
because of a failed unlink() call either.
- Heikki
