Date: Thu, 20 Mar 2025 13:16:44 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: md.c vs elog.c vs smgrreleaseall() in barrier
Message-ID: <20250320201644.3d.nmisch@google.com>
References: <3vae7l5ozvqtxmd7rr7zaeq3qkuipz365u3rtim5t5wdkr6f4g@vkgf2fogjirl>
 <CA+hUKG+HLL+Q6Q04R1UJc9fYo=z2ymZFHf0HJWat7-PdFCb+Gw@mail.gmail.com>
 <4qtmksxdbbp3pb7dqmn6lnzzdv7ujnizmbqtfbwm7c25waavtk@i6iyjrhk5eh5>
 <CA+hUKGJEckMw03WGV1fQ0zFyfp5SLdKFCuuKurkKCQHqdywCAw@mail.gmail.com>
 <kpopzahilmjtcpeefrsbrtzvwddzbhadsfn3cvdelnpi24gbgq@5ty2xbzoyua3>
 <20250319195553.ef.nmisch@google.com>
 <af3gdqg3guov3flyze2gjjphrrfbnusjesverlknn4tpxtafkh@vfpat6ffutek>
 <20250320004514.95.nmisch@google.com>
 <x6nmuzi5m6yzukp7lifuhfsnhyao3rse6aptatgg7llmz2gom6@h65qu63hqnli>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <x6nmuzi5m6yzukp7lifuhfsnhyao3rse6aptatgg7llmz2gom6@h65qu63hqnli>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250320201644.3d.nmisch%40google.com>
Precedence: bulk

On Thu, Mar 20, 2025 at 03:53:11PM -0400, Andres Freund wrote:
> I updated the patch with the following changes:
> 
> - Remove the assertion from smgrtruncate() - it would need to assert that it's
>   called in a critical section.
> 
>   Not sure why it's not already asserting that?
> 
>   The function header says:
>    * ... This function should normally
>    * be called in a critical section, but the current size must be checked
>    * outside the critical section, and no interrupts or smgr functions relating
>    * to this relation should be called in between.
> 
>   The "should normally" is bit weird imo, when would it be safe to *not* use
>   it in a critical section?

I expect it would be okay in recovery, which is a crypto-critical-section
IIRC.  All callers, including smgr_redo(), do have an explicit critical
section around the call.  Hence, I gather we're no longer relying on any
exceptions to this one's need for a critical section.

> - added comments about the reason for HOLD_INTERRUPTS to smgrdounlinkall(),
>   smgrdestroyall() and smgrreleaseall()

Perfect.

> I still am leaning against backpatching, but I'm not sure that's not just
> laziness.

It's also some risk reduction.  One of these smgr APIs might have a useful
interruptibility that we're now blocking.  (I'm not aware of one.)

> On 2025-03-19 17:45:14 -0700, Noah Misch wrote:
> > On Wed, Mar 19, 2025 at 06:45:20PM -0400, Andres Freund wrote:
> > > On 2025-03-19 12:55:53 -0700, Noah Misch wrote:
> > > > On Mon, Mar 17, 2025 at 07:52:02PM -0400, Andres Freund wrote:
> > > > > @@ -434,6 +481,8 @@ smgrdosyncall(SMgrRelation *rels, int nrels)
> > > > >  	if (nrels == 0)
> > > > >  		return;
> > > > >
> > > > > +	HOLD_INTERRUPTS();
> > > > > +
> > > > >  	FlushRelationsAllBuffers(rels, nrels);
> > > >
> > > > FlushRelationsAllBuffers() isn't part of smgr or md.c, so it's unlikely to
> > > > become sensitive to smgrrelease().  It may do a ton of I/O.  Hence, I'd
> > > > HOLD_INTERRUPTS() after FlushRelationsAllBuffers(), not before.
> > >
> > > Hm - we never would want to process interrupts while in
> > > FlushRelationsAllBuffers() or such, would we?  I'm ok with changing it, I
> > > guess I just didn't see a reason not to use a wider scope.
> >
> > If we get a query cancel or fast shutdown, it's better for the user to abort
> > the transaction rather than keep flushing.  smgrDoPendingSyncs() calls here
> > rather late in the pre-commit actions, so failing is still supposed to be
> > fine.  I think the code succeeds at making it fine to fail here.
> 
> But we don't actually intentionally accept interrupts in
> FlushRelationsAllBuffers()?

Yes.  It would be reasonable for future work to add that.

> It would only happen as a side-effect of a
> non-error elog/ereport() processing interrupts, right?

Likely yes.

> It also looks like we couldn't accept interrupts when called by
> AbortTransaction(), because there we already are in a HOLD_INTERRUPTS()
> region. I'm pretty sure an error would trigger at least an assertion. But
> that's really an independent issue.

The only smgrdosyncall() caller is smgrDoPendingSyncs(), which doesn't call it
in the abort case.  So I think we're good.

> Moved.

Thanks.

> > > I suspect it's always called with interrupts held already though.
> >
> > Ah, confirmed.  If I put this assert at the top of smgrdounlinkall(),
> > check-world passes:
> >
> > 	Assert(IsBinaryUpgrade || InRecovery || !INTERRUPTS_CAN_BE_PROCESSED());
> 
> I just made it hold interrupts for now, hope that makes sense?

Yep.


Patch looks perfect.