Thread: md.c vs elog.c vs smgrreleaseall() in barrier
Hi,
If there is any LOG/DEBUG elog inside functions like mdreadv(), mdwritev(),
mdextend(), be it directly or indirectly, the functions become unsafe. The
problem is that at the end of errfinish() there is a CFI:
/*
* Check for cancel/die interrupt first --- this is so that the user can
* stop a query emitting tons of notice or warning messages, even if it's
* in a loop that otherwise fails to check for interrupts.
*/
CHECK_FOR_INTERRUPTS();
that CFI in turn can call
ProcessProcSignalBarrier() ->
ProcessBarrierSmgrRelease() ->
smgrreleaseall()
which will free the MdfdVec that was opened at the start of mdreadv() etc.
I of course originally hit this with AIO, but it's not really AIO specific. If
I #define FDDEBUG I can quickly trigger crashes in master too. E.g.
(gdb) bt
#4 0x00005563a1bcb266 in ExceptionalCondition (conditionName=0x5563a10e2aae "FileIsValid(file)",
fileName=0x5563a10e24c8 "../../../../../home/andres/src/postgresql/src/backend/storage/file/fd.c",
lineNumber=2485)
at ../../../../../home/andres/src/postgresql/src/backend/utils/error/assert.c:66
#5 0x00005563a198d10b in FilePathName (file=-917820408) at
../../../../../home/andres/src/postgresql/src/backend/storage/file/fd.c:2485
#6 0x00005563a19cd40a in mdextend (reln=0x5563c952d568, forknum=MAIN_FORKNUM, blocknum=18, buffer=0x7fff13dda000,
skipFsync=true)
at ../../../../../home/andres/src/postgresql/src/backend/storage/smgr/md.c:505
#7 0x00005563a19d0848 in smgrextend (reln=0x5563c952d568, forknum=MAIN_FORKNUM, blocknum=18, buffer=0x7fff13dda000,
skipFsync=true)
at ../../../../../home/andres/src/postgresql/src/backend/storage/smgr/smgr.c:541
#8 0x00005563a1981800 in RelationCopyStorageUsingBuffer (srclocator=..., dstlocator=..., forkNum=MAIN_FORKNUM,
permanent=true)
at ../../../../../home/andres/src/postgresql/src/backend/storage/buffer/bufmgr.c:4667
#9 0x00005563a1981b8f in CreateAndCopyRelationData (src_rlocator=..., dst_rlocator=..., permanent=true)
at ../../../../../home/andres/src/postgresql/src/backend/storage/buffer/bufmgr.c:4760
#10 0x00005563a16561c7 in CreateDatabaseUsingWalLog (src_dboid=4, dst_dboid=1414794, src_tsid=1663, dst_tsid=1663)
at ../../../../../home/andres/src/postgresql/src/backend/commands/dbcommands.c:216
#11 0x00005563a16594de in createdb (pstate=0x5563c942f8b0, stmt=0x5563c9476fd8)
at ../../../../../home/andres/src/postgresql/src/backend/commands/dbcommands.c:1522
#12 0x00005563a19e05b2 in standard_ProcessUtility (pstmt=0x5563c9477068,
Right now I don't really see a solution other than putting
HOLD_INTERRUPTS()/RESUME_INTERRUPTS() into all those functions.
If we go for that, I can see an argument for doing that in smgr.c instead of
md.c, afaict any plausible smgr implementation would run into this, as the
smgrcloseall() is triggered from the smgr level.
Greetings,
Andres Freund
On Fri, Mar 14, 2025 at 5:03 AM Andres Freund <andres@anarazel.de> wrote: > If there is any LOG/DEBUG elog inside functions like mdreadv(), mdwritev(), > mdextend(), be it directly or indirectly, the functions become unsafe. The > problem is that at the end of errfinish() there is a CFI: > > /* > * Check for cancel/die interrupt first --- this is so that the user can > * stop a query emitting tons of notice or warning messages, even if it's > * in a loop that otherwise fails to check for interrupts. > */ > CHECK_FOR_INTERRUPTS(); > > that CFI in turn can call > ProcessProcSignalBarrier() -> > ProcessBarrierSmgrRelease() -> > smgrreleaseall() > > which will free the MdfdVec that was opened at the start of mdreadv() etc. > Right now I don't really see a solution other than putting > HOLD_INTERRUPTS()/RESUME_INTERRUPTS() into all those functions. Some other ideas: 1. An smgr-local smgr-release-hold flag, which would just cause ProcessBarrierSmgrReleaseAll() to return false. Main complication seems to be making sure you reset it on error, which is a bit annoying, elog.c probably needs to know how to clean it up. Yuck. 2. Somehow tell elog.c not to call CHECK_FOR_INTERRUPTS() instead. Also yuck, but at least elog.c is already the right place to clean state up on non-local exit... 3. Considering errfinish()'s stated goal, a sort of backstop to help you cancel looping message-spewing code only, I wonder if we should just restrict the kinds of interrupts it processes, so that it only calls CFI() if we're definitely throwing ERROR or FATAL? Something like CHECK_FOR_CANCEL_OR_DIE() which would enter the regular code only if it can already determine that, which would probably be defined as something like: if (INTERRUPTS_PENDING_CONDITION() && INTERRUPTS_CAN_BE_PROCESSED() && (QueryCancelPending || ProcDiePending)) ProcessInterrupts(). I realise that what I'm describing is essentially what CHECK_FOR_INTERRUPTS() used to be like, before a bunch of non-cancel, non-die stuff moved into CHECK_FOR_INTERRUPTS(), but that's probably the conditions under which that code was written. Could anything else be accidentally on elog() CFIs? Seems pretty weird? 4. Bigger refactoring of the interrupts system so you can express more precisely what kinds of stuff you want to handle. Well, we have some stuff like that coming in the nearby procsignal/interrupt refactoring thread. I don't feel enthusiastic about messing with it in the back branches, hence easier suggestion in #3. > If we go for that, I can see an argument for doing that in smgr.c instead of > md.c, afaict any plausible smgr implementation would run into this, as the > smgrcloseall() is triggered from the smgr level. Seems like maybe not a great idea to assume that future smgrs will be OK with being non-interruptible, if it can be avoided?
Hi,
On 2025-03-14 11:31:47 +1300, Thomas Munro wrote:
> 2. Somehow tell elog.c not to call CHECK_FOR_INTERRUPTS() instead.
> Also yuck, but at least elog.c is already the right place to clean
> state up on non-local exit...
How would that differ from HOLD_INTERRUPTS?
> 3. Considering errfinish()'s stated goal, a sort of backstop to help
> you cancel looping message-spewing code only, I wonder if we should
> just restrict the kinds of interrupts it processes, so that it only
> calls CFI() if we're definitely throwing ERROR or FATAL?
I'm not even sure it'd be safe to ERROR out in all the relevant places...
E.g.
/* 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);
doesn't this mean that ->pincount is uninitialized in case smgr_open() errors
out and that we'd loose track of the reln because it wasn't yet added to
unpinned_rels?
We should probably make sure it's safe against ERROR, given that there are
several paths towards that...
> Could anything else be accidentally on elog() CFIs? Seems pretty weird?
Hm, can't quite parse this...
> > If we go for that, I can see an argument for doing that in smgr.c instead of
> > md.c, afaict any plausible smgr implementation would run into this, as the
> > smgrcloseall() is triggered from the smgr level.
>
> Seems like maybe not a great idea to assume that future smgrs will be
> OK with being non-interruptible, if it can be avoided?
I think you'd need a fairly large surgery of smgr.c to make that viable - I
rather doubt that smgr.c itself is actually safe against interrupts.
I can see some arguable uses of smgr handling interrupts, say an smgr
implementation based on a network backed store, but you'd need rather massive
changes to actually be sure that smgr.c is called while accepting interrupts -
e.g. today sgmrwritev() will largely be called with interrupts held. Plenty
reads too. I doubt that undoing a few HOLD_INTERRUPTS() is a meaningful part
of the necessary work.
Greetings,
Andres Freund
On Fri, Mar 14, 2025 at 12:23 PM Andres Freund <andres@anarazel.de> wrote: > On 2025-03-14 11:31:47 +1300, Thomas Munro wrote: > > 2. Somehow tell elog.c not to call CHECK_FOR_INTERRUPTS() instead. > > Also yuck, but at least elog.c is already the right place to clean > > state up on non-local exit... > > How would that differ from HOLD_INTERRUPTS? Well I was just saying that if you try to make something similar you face the same problems, hence "yuck". The motivation for wanting to avoid full-scale HOLD_INTERRUPTS() is that there could be some other code path that does want interrupt processing of some limited kind, but it's all a bit hypothetical... > > 3. Considering errfinish()'s stated goal, a sort of backstop to help > > you cancel looping message-spewing code only, I wonder if we should > > just restrict the kinds of interrupts it processes, so that it only > > calls CFI() if we're definitely throwing ERROR or FATAL? > > I'm not even sure it'd be safe to ERROR out in all the relevant places... > > E.g. > /* 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); > > doesn't this mean that ->pincount is uninitialized in case smgr_open() errors > out and that we'd loose track of the reln because it wasn't yet added to > unpinned_rels? Ugh, right. > We should probably make sure it's safe against ERROR, given that there are > several paths towards that... Yeah. > > Could anything else be accidentally on elog() CFIs? Seems pretty weird? > > Hm, can't quite parse this... Sorry accidentally *depending* on. > > > If we go for that, I can see an argument for doing that in smgr.c instead of > > > md.c, afaict any plausible smgr implementation would run into this, as the > > > smgrcloseall() is triggered from the smgr level. > > > > Seems like maybe not a great idea to assume that future smgrs will be > > OK with being non-interruptible, if it can be avoided? > > I think you'd need a fairly large surgery of smgr.c to make that viable - I > rather doubt that smgr.c itself is actually safe against interrupts. > > I can see some arguable uses of smgr handling interrupts, say an smgr > implementation based on a network backed store, but you'd need rather massive > changes to actually be sure that smgr.c is called while accepting interrupts - > e.g. today sgmrwritev() will largely be called with interrupts held. Plenty > reads too. I doubt that undoing a few HOLD_INTERRUPTS() is a meaningful part > of the necessary work. Right, exactly the case I was thinking of: if some hypothetical future network smgr wants to be able to process *some* kinds of things carefully while waiting for the network. I don't think we want to constrain ourselves to NFS-style "pretend it's local and make it non-interruptible" without any escape hatches, but you're quite right that that's probably a whole research project of its own and we just haven't refined the interrupt system enough for that yet, so yeah I see how you arrived at that conclusion and it makes sense.
Hi, On 2025-03-14 12:57:51 +1300, Thomas Munro wrote: > On Fri, Mar 14, 2025 at 12:23 PM Andres Freund <andres@anarazel.de> wrote: > > > > 3. Considering errfinish()'s stated goal, a sort of backstop to help > > > you cancel looping message-spewing code only, I wonder if we should > > > just restrict the kinds of interrupts it processes, so that it only > > > calls CFI() if we're definitely throwing ERROR or FATAL? > > > > I'm not even sure it'd be safe to ERROR out in all the relevant places... > > > > E.g. > > /* 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); > > > > doesn't this mean that ->pincount is uninitialized in case smgr_open() errors > > out and that we'd loose track of the reln because it wasn't yet added to > > unpinned_rels? > > Ugh, right. Patch for that attached. > > > > If we go for that, I can see an argument for doing that in smgr.c instead of > > > > md.c, afaict any plausible smgr implementation would run into this, as the > > > > smgrcloseall() is triggered from the smgr level. > > > > > > Seems like maybe not a great idea to assume that future smgrs will be > > > OK with being non-interruptible, if it can be avoided? > > > > I think you'd need a fairly large surgery of smgr.c to make that viable - I > > rather doubt that smgr.c itself is actually safe against interrupts. > > > > I can see some arguable uses of smgr handling interrupts, say an smgr > > implementation based on a network backed store, but you'd need rather massive > > changes to actually be sure that smgr.c is called while accepting interrupts - > > e.g. today sgmrwritev() will largely be called with interrupts held. Plenty > > reads too. I doubt that undoing a few HOLD_INTERRUPTS() is a meaningful part > > of the necessary work. > > Right, exactly the case I was thinking of: if some hypothetical future > network smgr wants to be able to process *some* kinds of things > carefully while waiting for the network. I don't think we want to > constrain ourselves to NFS-style "pretend it's local and make it > non-interruptible" without any escape hatches, but you're quite right > that that's probably a whole research project of its own and we just > haven't refined the interrupt system enough for that yet, so yeah I > see how you arrived at that conclusion and it makes sense. Here's a proposed patch for this. It turns out that the bug might already be reachable, even without defining FDDEBUG. There's a debug ereport() in register_dirty_segment() - but it's hard to reach in practice. I don't really know whether that means we ought to backpatch or not - which makes me want to be conservative and not backpatch. Greetings, Andres Freund
Attachment
Hi, On 2025-03-17 19:52:02 -0400, Andres Freund wrote: > On 2025-03-14 12:57:51 +1300, Thomas Munro wrote: > > On Fri, Mar 14, 2025 at 12:23 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > 3. Considering errfinish()'s stated goal, a sort of backstop to help > > > > you cancel looping message-spewing code only, I wonder if we should > > > > just restrict the kinds of interrupts it processes, so that it only > > > > calls CFI() if we're definitely throwing ERROR or FATAL? > > > > > > I'm not even sure it'd be safe to ERROR out in all the relevant places... > > > > > > E.g. > > > /* 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); > > > > > > doesn't this mean that ->pincount is uninitialized in case smgr_open() errors > > > out and that we'd loose track of the reln because it wasn't yet added to > > > unpinned_rels? > > > > Ugh, right. > > Patch for that attached. The patch claimed: > This buglet was introduced in 21d9c3ee4ef7. As that commit is only in HEAD, no > need to backpatch. But looking at it again that's unfortunately not true. Turns out it's not 2024 anymore... Planning to backpatch that part to 17 soon. Greetings, Andres Freund
On Mon, Mar 17, 2025 at 07:52:02PM -0400, Andres Freund wrote:
> Here's a proposed patch for this. It turns out that the bug might already be
> reachable, even without defining FDDEBUG. There's a debug ereport() in
> register_dirty_segment() - but it's hard to reach in practice.
>
> I don't really know whether that means we ought to backpatch or not - which
> makes me want to be conservative and not backpatch.
Non-backpatch sounds fine.
> Subject: [PATCH v2 1/2] smgr: Hold interrupts in most smgr functions
> It seems better to put the HOLD_INTERRUPTS()/RESUME_INTERRUPTS() in smgr.c,
> instead of trying to push them down to md.c where possible: For one, every
> smgr implementation would be vulnerable, for another, a good bit of smgr.c
> code itself is affected too.
I'm fine with putting it in smgr.c. Academically, I don't see every smgr
implementation being vulnerable for most smgr entry points. For example, the
upthread backtrace happens because mdclose() undoes the fd-opening work of
mdnblocks(). Another smgr implementation could make its smgr_close callback a
no-op. smgrrelease() doesn't destroy anything important at the smgr level; it
is mdclose() that destroys state that md.c still needs.
> @@ -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.
> +
> hash_seq_init(&status, SMgrRelationHash);
>
> while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL)
> {
> smgrrelease(reln);
> }
> +
> + RESUME_INTERRUPTS();
> }
>
> /*
> @@ -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.
>
> /*
> @@ -449,6 +498,8 @@ smgrdosyncall(SMgrRelation *rels, int nrels)
> smgrsw[which].smgr_immedsync(rels[i], forknum);
Long-term, someone might change this to hold interrupts once per immedsync
with a CFI between immedsync calls. That would be safe. It's not this
change's job, though. I'm mentioning it for the archives.
> }
> }
> +
> + 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.
> 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?
Hi, On 2025-03-19 12:55:53 -0700, Noah Misch wrote: > On Mon, Mar 17, 2025 at 07:52:02PM -0400, Andres Freund wrote: > > Here's a proposed patch for this. It turns out that the bug might already be > > reachable, even without defining FDDEBUG. There's a debug ereport() in > > register_dirty_segment() - but it's hard to reach in practice. > > > > I don't really know whether that means we ought to backpatch or not - which > > makes me want to be conservative and not backpatch. > > Non-backpatch sounds fine. Cool. > > @@ -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. > > @@ -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. 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(). > > } > > } > > + > > + 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. But unfortunately I think this probably needs to be done in a critical section, not just run with interrupts held. We really really shouldn't ever palloc() after doing something like DropRelationsAllBuffers(). Thomas just spent a lot of time fixing corruption issues arising for related issues around relation truncations... I think this may mean that an error during smgr_unlink() leads to a cluster in a corrupted state? > > 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... Greetings, Andres Freund
Hi, On 2025-03-19 18:45:20 -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: > > > @@ -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. > > But unfortunately I think this probably needs to be done in a critical > section, not just run with interrupts held. > > We really really shouldn't ever palloc() after doing something like > DropRelationsAllBuffers(). Thomas just spent a lot of time fixing corruption > issues arising for related issues around relation truncations... > > I think this may mean that an error during smgr_unlink() leads to a cluster in > a corrupted state? Ah - it effectively is already in a critical section, just a weirdly spelled one: 2025-03-19 19:00:06.398 EDT [2156613][client backend][0/3:0][psql] LOG: statement: DROP TABLE foo; 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] ERROR: muahahaha 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] STATEMENT: DROP TABLE foo; 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] WARNING: AbortTransaction while in COMMIT state 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] PANIC: cannot abort transaction 43139, it was alreadycommitted Obviously not great, but better than corruption. Until the IO issue preventing smgrdounlinkall() to work are fixed, the DB doesn't start up again... 2025-03-19 19:00:57.711 EDT [2156761][startup][:0][] FATAL: muahahaha 2025-03-19 19:00:57.711 EDT [2156761][startup][:0][] CONTEXT: WAL redo at B/66D6E650 for Transaction/COMMIT: 2025-03-1919:00:06.400748-04; rels: base/5/25449; dropped stats: 2/5/25449; inval msgs: ... 2025-03-19 19:00:57.715 EDT [2156758][postmaster][:0][] LOG: startup process (PID 2156761) exited with exit code 1 2025-03-19 19:00:57.715 EDT [2156758][postmaster][:0][] LOG: terminating any other active server processes If one uses a large s_b and the system is busy, it's not even that unlikely that we would fail in that spot: mdunlinkfork()-> register_forget_request()-> RegisterSyncRequest()-> ForwardSyncRequest()-> CompactCheckpointerRequestQueue() can require a NBuffers * sizeof(bool) array and hashtable that fits all the entries... Greetings, Andres Freund
On Thu, Mar 20, 2025 at 12:06 PM Andres Freund <andres@anarazel.de> wrote: > Ah - it effectively is already in a critical section, just a weirdly spelled one: > > 2025-03-19 19:00:06.398 EDT [2156613][client backend][0/3:0][psql] LOG: statement: DROP TABLE foo; > 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] ERROR: muahahaha > 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] STATEMENT: DROP TABLE foo; > 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] WARNING: AbortTransaction while in COMMIT state > 2025-03-19 19:00:06.404 EDT [2156613][client backend][0/0:43139][psql] PANIC: cannot abort transaction 43139, it was alreadycommitted > > Obviously not great, but better than corruption. Yeah, I called that a crypto-critical-section over in this thread: https://www.postgresql.org/message-id/flat/ZYw8gVOMF9gfp6i5%40pryzbyj2023
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.
Hi, 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? - added comments about the reason for HOLD_INTERRUPTS to smgrdounlinkall(), smgrdestroyall() and smgrreleaseall() - moved the HOLD_INTERRUPTS after FlushRelationsAllBuffers() in smgrdosyncall - updated the comment & commit message talking about the problem being due to debug elogs/ereports - it's also LOG/WARNING. I was writing a remark in the commit message, explaining that the only < ERROR elog/ereport that can be reached is very unlikely to be reachable, and therefore it's not worth the risk of backpatching. But it turns out there also are WARNINGs in mdunlinkfork(), which seem a lot easier to reach than the DEBUG1 in register_dirty_segment(). I still am leaning against backpatching, but I'm not sure that's not just laziness. 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()? It would only happen as a side-effect of a non-error elog/ereport() processing interrupts, right? 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. Moved. > > > > @@ -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()); I just made it hold interrupts for now, hope that makes sense? > > > 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. FWIW, if mdopen() could fail, it should probably only do fallible operations after doing the non-fallible initialization. Then mdnblocks() wouldn't need to cure anything. Greetings, Andres Freund
Attachment
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.
Hi, On 2025-03-20 13:16:44 -0700, Noah Misch wrote: > Patch looks perfect. Thanks for the reviews! Pushed. Greetings, Andres Freund