From 020ccd14b177494a7d7e00ea3ef50f400a9b1032 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 10 Sep 2024 12:04:38 +1200 Subject: [PATCH v4] RelationTruncate() must use a critical section. RelationTruncate() does three things, while holding an AccessExclusiveLock and preventing checkpoints: 1. Logs the truncation. 2. Drops buffers, even if they're dirty. 3. Truncates files, possibly more than one. Step 3 might fail with an operating system error, but we're already committed to the operation at this point. Previously the theory expressed in comments was that crash recovery would surely fail to truncate again, so we should avoid a PANIC loop. That didn't account for the seriousness of the failure mode: dead tuples could come back to life, because we threw away needed dirty buffers, and replicas could PANIC, because they replayed the truncation but then later they might see references to blocks they consider to be past the end. This commit wraps the whole operation in a critical section, so that any error triggers a PANIC. In order to make that possible, it provides smgrtruncatefrom(), so that we can move the smgrnblocks() calls out of the critical section, and adjusts md.c to defer reallocating its internal array of segments when its size decreases. This addresses bug #18146, an ancient problem mostly affecting Windows in practice, but also bug #18426 which revealed a new way to reach a partially truncated state even on Unix since PostgreSQL 14. Commit d8725104 made it possible for WaitIO() reached via DropRelationBuffers() to be interrupted, but that's no longer possible as a side-effect of the critical section. This issue has been independently analyzed by a large number of people and threads over the years, most recently Robert Haas, Michael Paquier and myself. Ideas for how to fix with new buffer states have been mooted, but that wouldn't be back-patchable. Reviewed-by: Michael Paquier Reported-by: rootcause000@gmail.com Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org Discussion: https://postgr.es/m/2348.1544474335@sss.pgh.pa.us Discussion: https://postgr.es/m/5BBC590AE8DF4ED1A170E4D48F1B53AC%40tunaPC --- src/backend/catalog/storage.c | 31 +++++++++++++++++++++++++------ src/backend/storage/smgr/md.c | 27 +++++++++++++++++++-------- src/backend/storage/smgr/smgr.c | 25 +++++++++++++++++++++++-- src/include/storage/md.h | 2 +- src/include/storage/smgr.h | 3 +++ 5 files changed, 71 insertions(+), 17 deletions(-) diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c index f56b3cc0f23..eefe13aba08 100644 --- a/src/backend/catalog/storage.c +++ b/src/backend/catalog/storage.c @@ -291,6 +291,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) bool vm; bool need_fsm_vacuum = false; ForkNumber forks[MAX_FORKNUM]; + BlockNumber old_blocks[MAX_FORKNUM]; BlockNumber blocks[MAX_FORKNUM]; int nforks = 0; SMgrRelation reln; @@ -306,6 +307,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) /* Prepare for truncation of MAIN fork of the relation */ forks[nforks] = MAIN_FORKNUM; + old_blocks[nforks] = smgrnblocks(reln, MAIN_FORKNUM); blocks[nforks] = nblocks; nforks++; @@ -316,6 +318,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) blocks[nforks] = FreeSpaceMapPrepareTruncateRel(rel, nblocks); if (BlockNumberIsValid(blocks[nforks])) { + old_blocks[nforks] = smgrnblocks(reln, FSM_FORKNUM); forks[nforks] = FSM_FORKNUM; nforks++; need_fsm_vacuum = true; @@ -329,6 +332,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) blocks[nforks] = visibilitymap_prepare_truncate(rel, nblocks); if (BlockNumberIsValid(blocks[nforks])) { + old_blocks[nforks] = smgrnblocks(reln, VISIBILITYMAP_FORKNUM); forks[nforks] = VISIBILITYMAP_FORKNUM; nforks++; } @@ -355,12 +359,25 @@ RelationTruncate(Relation rel, BlockNumber nblocks) /* * We WAL-log the truncation before actually truncating, which means * trouble if the truncation fails. If we then crash, the WAL replay - * likely isn't going to succeed in the truncation either, and cause a - * PANIC. It's tempting to put a critical section here, but that cure - * would be worse than the disease. It would turn a usually harmless - * failure to truncate, that might spell trouble at WAL replay, into a - * certain PANIC. + * likely isn't going to succeed in the truncation either, so it's + * tempting not to put a critical section here to avoid a double PANIC. + * However, if the file system operation failed after + * DropRelationBuffers() had already thrown away dirty buffers, (1) old + * deleted data might later come back to life, and (2) would-be-truncated + * pages might be modified again, and then a replica that had replayed the + * truncation would PANIC because it has a different idea of the relation + * size. + * + * The critical section also makes WaitIO() non-interruptable in + * DropRelationBuffers(), which would otherwise be another way to reach + * the above problems. + * + * Note that we capture the current size before entering the critical + * section, so that we can use smgrtruncatefrom(). smgrtruncate() can't + * be used in a critical section because it might allocate memory. */ + START_CRIT_SECTION(); + if (RelationNeedsWAL(rel)) { /* @@ -395,7 +412,9 @@ RelationTruncate(Relation rel, BlockNumber nblocks) * longer exist after truncation is complete, and then truncate the * corresponding files on disk. */ - smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks); + smgrtruncatefrom(RelationGetSmgr(rel), forks, nforks, old_blocks, blocks); + + END_CRIT_SECTION(); /* We've done all the critical work, so checkpoints are OK now. */ MyProc->delayChkptFlags &= ~DELAY_CHKPT_COMPLETE; diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 6796756358f..ab7fd58a2c7 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -1141,19 +1141,20 @@ mdnblocks(SMgrRelation reln, ForkNumber forknum) /* * mdtruncate() -- Truncate relation to specified number of blocks. + * + * Guaranteed not to allocate memory, so it can be used in a critical section. + * Caller must have called smgrnblocks() to obtain curnblk while holding a + * sufficient lock to prevent a change in relation size. This also makes sure + * we have opened all active segments, so that truncate loop will get them + * all! That step can't be done in a critical section. */ void -mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks) +mdtruncate(SMgrRelation reln, ForkNumber forknum, + BlockNumber curnblk, BlockNumber nblocks) { - BlockNumber curnblk; BlockNumber priorblocks; int curopensegs; - /* - * NOTE: mdnblocks makes sure we have opened all active segments, so that - * truncation loop will get them all! - */ - curnblk = mdnblocks(reln, forknum); if (nblocks > curnblk) { /* Bogus request ... but no complaint if InRecovery */ @@ -1492,7 +1493,7 @@ _fdvec_resize(SMgrRelation reln, reln->md_seg_fds[forknum] = MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg); } - else + else if (nseg > reln->md_num_open_segs[forknum]) { /* * It doesn't seem worthwhile complicating the code to amortize @@ -1504,6 +1505,16 @@ _fdvec_resize(SMgrRelation reln, repalloc(reln->md_seg_fds[forknum], sizeof(MdfdVec) * nseg); } + else + { + /* + * We don't reallocate a smaller array, because we want mdtruncate() + * to be able to promise that it won't allocate memory, so that it is + * allowed in a critical section. This means that a bit of space in + * the array is now wasted, until the next time we add a segment and + * reallocate. + */ + } reln->md_num_open_segs[forknum] = nseg; } diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index 7b9fa103eff..2aef34c4e9a 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -99,7 +99,7 @@ typedef struct f_smgr BlockNumber blocknum, BlockNumber nblocks); BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum); void (*smgr_truncate) (SMgrRelation reln, ForkNumber forknum, - BlockNumber nblocks); + BlockNumber old_blocks, BlockNumber nblocks); void (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum); void (*smgr_registersync) (SMgrRelation reln, ForkNumber forknum); } f_smgr; @@ -701,6 +701,26 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum) */ void smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nblocks) +{ + BlockNumber old_nblocks[MAX_FORKNUM + 1]; + + for (int i = 0; i < nforks; ++i) + old_nblocks[i] = smgrnblocks(reln, forknum[i]); + + return smgrtruncatefrom(reln, forknum, nforks, old_nblocks, nblocks); +} + +/* + * smgrtruncatefrom() -- Like sgmrtruncate(), but with caller-supplied old + * sizes, to allow usage in critical sections. + * + * See smgrtruncate() for requirements, and additionally the caller must call + * smgrnblocks() to obtain the size of each fork to be truncated, and not + * allow the relation to be invalidated in between. + */ +void +smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum, int nforks, + BlockNumber *old_nblocks, BlockNumber *nblocks) { int i; @@ -728,7 +748,8 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nb /* Make the cached size is invalid if we encounter an error. */ reln->smgr_cached_nblocks[forknum[i]] = InvalidBlockNumber; - smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i], nblocks[i]); + smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i], + old_nblocks[i], nblocks[i]); /* * We might as well update the local smgr_cached_nblocks values. The diff --git a/src/include/storage/md.h b/src/include/storage/md.h index 620f10abdeb..5dbd0033c6f 100644 --- a/src/include/storage/md.h +++ b/src/include/storage/md.h @@ -41,7 +41,7 @@ extern void mdwriteback(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, BlockNumber nblocks); extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum); extern void mdtruncate(SMgrRelation reln, ForkNumber forknum, - BlockNumber nblocks); + BlockNumber old_blocks, BlockNumber nblocks); extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum); extern void mdregistersync(SMgrRelation reln, ForkNumber forknum); diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h index e15b20a566a..2dc146dc07d 100644 --- a/src/include/storage/smgr.h +++ b/src/include/storage/smgr.h @@ -105,6 +105,9 @@ extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum); extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum); extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nblocks); +extern void smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum, + int nforks, + BlockNumber *old_nblocks, BlockNumber *nblocks); extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum); extern void smgrregistersync(SMgrRelation reln, ForkNumber forknum); extern void AtEOXact_SMgr(void); -- 2.46.0