Hi,
On 2023-02-21 17:40:31 +0200, Heikki Linnakangas wrote:
> > v2-0006-bufmgr-Support-multiple-in-progress-IOs-by-using-.patch
>
> This looks straightforward. My only concern is that it changes the order
> that things happen at abort. Currently, AbortBufferIO() is called very early
> in AbortTransaction(), and this patch moves it much later. I don't see any
> immediate problems from that, but it feels scary.
Yea, it does feel a bit awkward. But I suspect it's actually the right
thing. We've not even adjusted the transaction state at the point we're
calling AbortBufferIO(). And AbortBufferIO() will sometimes allocate memory
for a WARNING, which conceivably could fail - although I don't think that's a
particularly realistic scenario due to TransactionAbortContext (I guess you
could have a large error context stack or such).
Medium term I think we need to move a lot more of the error handling into
resowners. Having a dozen+ places with their own choreographed sigsetjmp()
recovery blocks is error prone as hell. Not to mention tedious.
> > @@ -2689,7 +2685,6 @@ InitBufferPoolAccess(void)
> > static void
> > AtProcExit_Buffers(int code, Datum arg)
> > {
> > - AbortBufferIO();
> > UnlockBuffers();
> > CheckForBufferLeaks();
>
> Hmm, do we call AbortTransaction() and ResourceOwnerRelease() on
> elog(FATAL)? Do we need to worry about that?
We have before_shmem_exit() callbacks that should protect against
that. InitPostgres() registers ShutdownPostgres(), and
CreateAuxProcessResourceOwner() registers
ReleaseAuxProcessResourcesCallback().
I think we'd already be in trouble if we didn't reliably end up doing resowner
cleanup during process exit.
Perhaps ResourceOwnerCreate()/ResourceOwnerDelete() should maintain a list of
"active" resource owners and have a before-exit callback that ensures the list
is empty and PANICs if not? Better a crash restart than hanging because we
didn't release some shared resource.
Greetings,
Andres Freund
