Re: md.c vs elog.c vs smgrreleaseall() in barrier

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:
> > > @@ -362,12 +397,16 @@ smgrreleaseall(void)
> > > if (SMgrRelationHash == NULL)
> > > return;
> > >
> > > + HOLD_INTERRUPTS();
> >
> > Likely not important, but it's not clear to me why smgrdestroyall() and
> > smgrreleaseall() get HOLD_INTERRUPTS(), as opposed to relying on the holds in
> > smgrdestroy() and smgrrelease(). In contrast, smgrreleaserellocator() does
> > rely on smgrrelease() for the hold.
>
> It didn't seem particularly safe to allow interrupts, which in turn could
> change the list of open relations, while iterating over a linked list / a
> hashtable.

Fair.

> > > @@ -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.

> I guess I am a bit paranoid because gave me flashbacks to issues around
> smgrtruncate() failing after doing DropRelationBuffers(), that Thomas recently
> fixed (and I had worked on a few years before). But of course
> DropRelationBuffers() is way more dangerous than FlushRelationsAllBuffers().

Fair.

> > > }
> > > }
> > > +
> > > + RESUME_INTERRUPTS();
> > > }
> > >
> > > /*
> > > @@ -471,6 +522,8 @@ smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo)
> > > if (nrels == 0)
> > > return;
> > >
> > > + HOLD_INTERRUPTS();
> >
> > I would move this below DropRelationsAllBuffers(), for reasons like
> > FlushRelationsAllBuffers() above.
>
> I think that'd be unsafe. Once we dropped buffers from the buffer pool we
> can't just continue without also unlinking the underlying relation, otherwise
> an older view of the data later can be "revived from the dead" after an error,
> causing all manner of corruption.
>
> 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());

> > > Subject: [PATCH v2 2/2] smgr: Make SMgrRelation initialization safer against
> > > errors
> > >
> > > In case the smgr_open callback failed, the ->pincount field would not be
> > > initialized and the relation would not be put onto the unpinned_relns list.
> > >
> > > This buglet was introduced in 21d9c3ee4ef7. As that commit is only in HEAD, no
> > > need to backpatch.
> > >
> > > Discussion: https://postgr.es/m/3vae7l5ozvqtxmd7rr7zaeq3qkuipz365u3rtim5t5wdkr6f4g@vkgf2fogjirl
> > > ---
> > > src/backend/storage/smgr/smgr.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
> > > index 53a09fe4aaa..24971304b85 100644
> > > --- a/src/backend/storage/smgr/smgr.c
> > > +++ b/src/backend/storage/smgr/smgr.c
> > > @@ -255,12 +255,12 @@ smgropen(RelFileLocator rlocator, ProcNumber backend)
> > > reln->smgr_cached_nblocks[i] = InvalidBlockNumber;
> > > reln->smgr_which = 0; /* we only have md.c at present */
> > >
> > > - /* implementation-specific initialization */
> > > - smgrsw[reln->smgr_which].smgr_open(reln);
> > > -
> > > /* it is not pinned yet */
> > > reln->pincount = 0;
> > > dlist_push_tail(&unpinned_relns, &reln->node);
> > > +
> > > + /* implementation-specific initialization */
> > > + smgrsw[reln->smgr_which].smgr_open(reln);
> > > }
> >
> > I struggle to speculate about the merits of this, because mdopen() can't fail.
> > If mdopen() started to do things that could fail, mdnblocks() would be
> > reasonable in assuming those things are done. Hence, the long-term direction
> > should be more like destroying the new smgr entry in the event of error.
> >
> > I would not make this change. I'd maybe add a comment that smgr_open
> > callbacks currently aren't allowed to fail, since smgropen() isn't ready to
> > clean up smgr-level state from a failed open. How do you see it?
>
> I see no disadvantage in the change - it seems strictly better to initialize
> pincount earlier. I agree that it might be a good idea to explicitly handle
> errors eventually, but that'd not be made harder by this change...

Okay. I suppose if mdopen() gained the ability to fail and mdnblocks() also
gained the ability to cure said failure, this change would make that okay.