From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: md.c vs elog.c vs smgrreleaseall() in barrier
Date: Thu, 20 Mar 2025 13:16:44 -0700
Message-ID: <20250320201644.3d.nmisch@google.com> (raw)
In-Reply-To: <x6nmuzi5m6yzukp7lifuhfsnhyao3rse6aptatgg7llmz2gom6@h65qu63hqnli>
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>
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.
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: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, andres@anarazel.de, thomas.munro@gmail.com
Subject: Re: md.c vs elog.c vs smgrreleaseall() in barrier
In-Reply-To: <20250320201644.3d.nmisch@google.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox