public inbox for [email protected]  

help / color / mirror / Atom feed

From: Andres Freund <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: vignesh C <[email protected]>
Cc: [email protected]
Cc: Thomas Munro <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: Yura Sokolov <[email protected]>
Cc: Robert Haas <[email protected]>
Subject: Re: refactoring relation extension and BufferAlloc(), faster COPY
Date: Tue, 21 Feb 2023 10:42:09 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CALDaNm3prG-F9DS2G8od9QyrE5HefMvmk5oqvEs64n+tPbkd8g@mail.gmail.com>
	<[email protected]>
	<[email protected]>

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

view thread (3+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: refactoring relation extension and BufferAlloc(), faster COPY
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox