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