From da8a868fd73f96604426a232f8300f50bc26e425 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sat, 15 Mar 2025 12:29:57 -0400 Subject: [PATCH v2.9 07/30] smgr: Hold interrupts in most smgr functions We need to hold interrupts across most of the smgr.c/md.c functions, as otherwise interrupt processing, e.g. due to a debug elog/ereport, can trigger procsignal processing, which in turn can trigger smgrreleaseall(). As the relevant code is not reentrant we quickly end up in a bad situation. 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. Discussion: https://postgr.es/m/3vae7l5ozvqtxmd7rr7zaeq3qkuipz365u3rtim5t5wdkr6f4g@vkgf2fogjirl --- src/backend/storage/smgr/smgr.c | 94 +++++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index ebe35c04de5..8787ce9b18f 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -40,6 +40,15 @@ * themselves, as there could pointers to them in active use. See * smgrrelease() and smgrreleaseall(). * + * NB: We need to hold interrupts across most of the functions in this file, + * as otherwise interrupt processing, e.g. due to a debug elog/ereport, can + * trigger procsignal processing, which in turn can trigger + * smgrreleaseall(). None of the relevant code is reentrant. It seems better + * to put the HOLD_INTERRUPTS()/RESUME_INTERRUPTS() here, 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. + * * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * @@ -53,6 +62,7 @@ #include "access/xlogutils.h" #include "lib/ilist.h" +#include "miscadmin.h" #include "storage/bufmgr.h" #include "storage/ipc.h" #include "storage/md.h" @@ -158,12 +168,16 @@ smgrinit(void) { int i; + HOLD_INTERRUPTS(); + for (i = 0; i < NSmgr; i++) { if (smgrsw[i].smgr_init) smgrsw[i].smgr_init(); } + RESUME_INTERRUPTS(); + /* register the shutdown proc */ on_proc_exit(smgrshutdown, 0); } @@ -176,11 +190,13 @@ smgrshutdown(int code, Datum arg) { int i; + HOLD_INTERRUPTS(); for (i = 0; i < NSmgr; i++) { if (smgrsw[i].smgr_shutdown) smgrsw[i].smgr_shutdown(); } + RESUME_INTERRUPTS(); } /* @@ -206,6 +222,8 @@ smgropen(RelFileLocator rlocator, ProcNumber backend) Assert(RelFileNumberIsValid(rlocator.relNumber)); + HOLD_INTERRUPTS(); + if (SMgrRelationHash == NULL) { /* First time through: initialize the hash table */ @@ -242,6 +260,8 @@ smgropen(RelFileLocator rlocator, ProcNumber backend) dlist_push_tail(&unpinned_relns, &reln->node); } + RESUME_INTERRUPTS(); + return reln; } @@ -283,6 +303,8 @@ smgrdestroy(SMgrRelation reln) Assert(reln->pincount == 0); + HOLD_INTERRUPTS(); + for (forknum = 0; forknum <= MAX_FORKNUM; forknum++) smgrsw[reln->smgr_which].smgr_close(reln, forknum); @@ -292,6 +314,8 @@ smgrdestroy(SMgrRelation reln) &(reln->smgr_rlocator), HASH_REMOVE, NULL) == NULL) elog(ERROR, "SMgrRelation hashtable corrupted"); + + RESUME_INTERRUPTS(); } /* @@ -302,12 +326,16 @@ smgrdestroy(SMgrRelation reln) void smgrrelease(SMgrRelation reln) { + HOLD_INTERRUPTS(); + for (ForkNumber forknum = 0; forknum <= MAX_FORKNUM; forknum++) { smgrsw[reln->smgr_which].smgr_close(reln, forknum); reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber; } reln->smgr_targblock = InvalidBlockNumber; + + RESUME_INTERRUPTS(); } /* @@ -336,6 +364,8 @@ smgrdestroyall(void) { dlist_mutable_iter iter; + HOLD_INTERRUPTS(); + /* * Zap all unpinned SMgrRelations. We rely on smgrdestroy() to remove * each one from the list. @@ -347,6 +377,8 @@ smgrdestroyall(void) smgrdestroy(rel); } + + RESUME_INTERRUPTS(); } /* @@ -362,12 +394,16 @@ smgrreleaseall(void) if (SMgrRelationHash == NULL) return; + HOLD_INTERRUPTS(); + hash_seq_init(&status, SMgrRelationHash); while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL) { smgrrelease(reln); } + + RESUME_INTERRUPTS(); } /* @@ -400,7 +436,13 @@ smgrreleaserellocator(RelFileLocatorBackend rlocator) bool smgrexists(SMgrRelation reln, ForkNumber forknum) { - return smgrsw[reln->smgr_which].smgr_exists(reln, forknum); + bool ret; + + HOLD_INTERRUPTS(); + ret = smgrsw[reln->smgr_which].smgr_exists(reln, forknum); + RESUME_INTERRUPTS(); + + return ret; } /* @@ -413,7 +455,9 @@ smgrexists(SMgrRelation reln, ForkNumber forknum) void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo) { + HOLD_INTERRUPTS(); smgrsw[reln->smgr_which].smgr_create(reln, forknum, isRedo); + RESUME_INTERRUPTS(); } /* @@ -434,6 +478,8 @@ smgrdosyncall(SMgrRelation *rels, int nrels) if (nrels == 0) return; + HOLD_INTERRUPTS(); + FlushRelationsAllBuffers(rels, nrels); /* @@ -449,6 +495,8 @@ smgrdosyncall(SMgrRelation *rels, int nrels) smgrsw[which].smgr_immedsync(rels[i], forknum); } } + + RESUME_INTERRUPTS(); } /* @@ -471,6 +519,8 @@ smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo) if (nrels == 0) return; + HOLD_INTERRUPTS(); + /* * Get rid of any remaining buffers for the relations. bufmgr will just * drop them without bothering to write the contents. @@ -522,6 +572,8 @@ smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo) } pfree(rlocators); + + RESUME_INTERRUPTS(); } @@ -538,6 +590,8 @@ void smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const void *buffer, bool skipFsync) { + HOLD_INTERRUPTS(); + smgrsw[reln->smgr_which].smgr_extend(reln, forknum, blocknum, buffer, skipFsync); @@ -550,6 +604,8 @@ smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, reln->smgr_cached_nblocks[forknum] = blocknum + 1; else reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber; + + RESUME_INTERRUPTS(); } /* @@ -563,6 +619,8 @@ void smgrzeroextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, int nblocks, bool skipFsync) { + HOLD_INTERRUPTS(); + smgrsw[reln->smgr_which].smgr_zeroextend(reln, forknum, blocknum, nblocks, skipFsync); @@ -575,6 +633,8 @@ smgrzeroextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, reln->smgr_cached_nblocks[forknum] = blocknum + nblocks; else reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber; + + RESUME_INTERRUPTS(); } /* @@ -588,7 +648,13 @@ bool smgrprefetch(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, int nblocks) { - return smgrsw[reln->smgr_which].smgr_prefetch(reln, forknum, blocknum, nblocks); + bool ret; + + HOLD_INTERRUPTS(); + ret = smgrsw[reln->smgr_which].smgr_prefetch(reln, forknum, blocknum, nblocks); + RESUME_INTERRUPTS(); + + return ret; } /* @@ -601,7 +667,13 @@ uint32 smgrmaxcombine(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum) { - return smgrsw[reln->smgr_which].smgr_maxcombine(reln, forknum, blocknum); + uint32 ret; + + HOLD_INTERRUPTS(); + ret = smgrsw[reln->smgr_which].smgr_maxcombine(reln, forknum, blocknum); + RESUME_INTERRUPTS(); + + return ret; } /* @@ -619,8 +691,10 @@ void smgrreadv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, void **buffers, BlockNumber nblocks) { + HOLD_INTERRUPTS(); smgrsw[reln->smgr_which].smgr_readv(reln, forknum, blocknum, buffers, nblocks); + RESUME_INTERRUPTS(); } /* @@ -653,8 +727,10 @@ void smgrwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const void **buffers, BlockNumber nblocks, bool skipFsync) { + HOLD_INTERRUPTS(); smgrsw[reln->smgr_which].smgr_writev(reln, forknum, blocknum, buffers, nblocks, skipFsync); + RESUME_INTERRUPTS(); } /* @@ -665,8 +741,10 @@ void smgrwriteback(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, BlockNumber nblocks) { + HOLD_INTERRUPTS(); smgrsw[reln->smgr_which].smgr_writeback(reln, forknum, blocknum, nblocks); + RESUME_INTERRUPTS(); } /* @@ -683,10 +761,14 @@ smgrnblocks(SMgrRelation reln, ForkNumber forknum) if (result != InvalidBlockNumber) return result; + HOLD_INTERRUPTS(); + result = smgrsw[reln->smgr_which].smgr_nblocks(reln, forknum); reln->smgr_cached_nblocks[forknum] = result; + RESUME_INTERRUPTS(); + return result; } @@ -731,6 +813,8 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, { int i; + Assert(!INTERRUPTS_CAN_BE_PROCESSED()); + /* * Get rid of any buffers for the about-to-be-deleted blocks. bufmgr will * just drop them without bothering to write the contents. @@ -784,7 +868,9 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, void smgrregistersync(SMgrRelation reln, ForkNumber forknum) { + HOLD_INTERRUPTS(); smgrsw[reln->smgr_which].smgr_registersync(reln, forknum); + RESUME_INTERRUPTS(); } /* @@ -816,7 +902,9 @@ smgrregistersync(SMgrRelation reln, ForkNumber forknum) void smgrimmedsync(SMgrRelation reln, ForkNumber forknum) { + HOLD_INTERRUPTS(); smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum); + RESUME_INTERRUPTS(); } /* -- 2.48.1.76.g4e746b1a31.dirty