From 19890fda4fb9e207c3b7dea3ae77c322227c5695 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Fri, 6 Sep 2024 10:34:14 +1200 Subject: [PATCH v3 3/3] 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 smgrpreparetruncate() so that smgrtruncate() can promise not to call palloc() (banned in critical sections). This involves using GetRelationPathInPlace() instead of GetRelationPath(), and adjust 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 and myself. Ideas for how to fix with new buffer states have been proposed 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 | 28 +++++++++--- src/backend/storage/smgr/md.c | 75 +++++++++++++++++++++------------ src/backend/storage/smgr/smgr.c | 12 ++++++ src/include/storage/smgr.h | 1 + 4 files changed, 84 insertions(+), 32 deletions(-) diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c index bb8c4d15612..dabaa48a57b 100644 --- a/src/backend/catalog/storage.c +++ b/src/backend/catalog/storage.c @@ -308,6 +308,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) forks[nforks] = MAIN_FORKNUM; blocks[nforks] = nblocks; nforks++; + smgrpreparetruncate(reln, MAIN_FORKNUM); /* Prepare for truncation of the FSM if it exists */ fsm = smgrexists(RelationGetSmgr(rel), FSM_FORKNUM); @@ -320,6 +321,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) nforks++; need_fsm_vacuum = true; } + smgrpreparetruncate(reln, FSM_FORKNUM); } /* Prepare for truncation of the visibility map too if it exists */ @@ -332,6 +334,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks) forks[nforks] = VISIBILITYMAP_FORKNUM; nforks++; } + smgrpreparetruncate(reln, VISIBILITYMAP_FORKNUM); } RelationPreTruncate(rel); @@ -368,12 +371,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 called smgrpreparetruncate() above, to allow + * smgrtruncate() to be called within a critical section without + * allocating memory. */ + START_CRIT_SECTION(); + if (RelationNeedsWAL(rel)) { /* @@ -410,6 +426,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks) */ smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks); + END_CRIT_SECTION(); + /* We've done all the critical work, so checkpoints are OK now. */ MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE); diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 6796756358f..c01b1f74875 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -117,6 +117,12 @@ static MemoryContext MdCxt; /* context for all MdfdVec objects */ /* don't try to open a segment, if not already open */ #define EXTENSION_DONT_OPEN (1 << 5) +/* + * The maximum possible size of the .NNN extension for segments is 1 for the + * dot plus log10(2^32) for the digits for RELSEG_SIZE = 1, and it's not worth + * working harder to compute the value for more typical RELSEG_SIZE values. + */ +#define MAX_EXTENSION_SIZE (1 + 10) /* local routines */ static void mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, @@ -131,7 +137,7 @@ static void register_forget_request(RelFileLocatorBackend rlocator, ForkNumber f static void _fdvec_resize(SMgrRelation reln, ForkNumber forknum, int nseg); -static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, +static char *_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum, BlockNumber segno); static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno, int oflags); @@ -408,7 +414,8 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) */ if (ret >= 0 || errno != ENOENT) { - char *segpath = (char *) palloc(strlen(path) + 12); + char *segpath = (char *) palloc(strlen(path) + + MAX_EXTENSION_SIZE + 1); BlockNumber segno; for (segno = 1;; segno++) @@ -1492,7 +1499,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,31 +1511,50 @@ _fdvec_resize(SMgrRelation reln, repalloc(reln->md_seg_fds[forknum], sizeof(MdfdVec) * nseg); } + else + { + /* + * We don't reallocate a smaller array, because we want + * smgrpreparetruncate() to be able to promise that smgrtruncate() + * won't call memory allocation functions that would fail 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; } /* - * Return the filename for the specified segment of the relation. The - * returned string is palloc'd. + * Return the filename for the specified segment of the relation. The output + * buffer must be of size MAXPGPATH. */ static char * -_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno) +_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum, BlockNumber segno) { - char *path, - *fullpath; + size_t size; - path = relpath(reln->smgr_rlocator, forknum); + size = GetRelationPathInPlace(path, + reln->smgr_rlocator.locator.dbOid, + reln->smgr_rlocator.locator.spcOid, + reln->smgr_rlocator.locator.relNumber, + reln->smgr_rlocator.backend, + forknum); + /* + * Make sure the whole path, maximum possible segment suffix and NUL + * terminator can fit into MAXPGPATH. + */ + if (size + MAX_EXTENSION_SIZE >= MAXPGPATH) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("path \"%s\" too long", path))); + + /* Append segment number. */ if (segno > 0) - { - fullpath = psprintf("%s.%u", path, segno); - pfree(path); - } - else - fullpath = path; + sprintf(path + size, ".%u", segno); - return fullpath; + return path; } /* @@ -1541,15 +1567,13 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno, { MdfdVec *v; File fd; - char *fullpath; + char fullpath[MAXPGPATH]; - fullpath = _mdfd_segpath(reln, forknum, segno); + _mdfd_segpath(fullpath, reln, forknum, segno); /* open the file */ fd = PathNameOpenFile(fullpath, _mdfd_open_flags() | oflags); - pfree(fullpath); - if (fd < 0) return NULL; @@ -1584,6 +1608,7 @@ static MdfdVec * _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, bool skipFsync, int behavior) { + char path[MAXPGPATH]; MdfdVec *v; BlockNumber targetseg; BlockNumber nextsegno; @@ -1686,7 +1711,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, ereport(ERROR, (errcode_for_file_access(), errmsg("could not open file \"%s\" (target block %u): previous segment is only %u blocks", - _mdfd_segpath(reln, forknum, nextsegno), + _mdfd_segpath(path, reln, forknum, nextsegno), blkno, nblocks))); } @@ -1700,7 +1725,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno, ereport(ERROR, (errcode_for_file_access(), errmsg("could not open file \"%s\" (target block %u): %m", - _mdfd_segpath(reln, forknum, nextsegno), + _mdfd_segpath(path, reln, forknum, nextsegno), blkno))); } } @@ -1751,11 +1776,7 @@ mdsyncfiletag(const FileTag *ftag, char *path) } else { - char *p; - - p = _mdfd_segpath(reln, ftag->forknum, ftag->segno); - strlcpy(path, p, MAXPGPATH); - pfree(p); + _mdfd_segpath(path, reln, ftag->forknum, ftag->segno); file = PathNameOpenFile(path, _mdfd_open_flags()); if (file < 0) diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index 7b9fa103eff..323e5114f6a 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -689,6 +689,18 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum) return InvalidBlockNumber; } +/* + * smgrpreparetruncate() -- Prepare to truncate a fork of a relation. + * + * This promises that a later call to smgrtruncate() will not have to allocate + * memory from MemoryContexts that don't allow that inside critical sections. + */ +void +smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum) +{ + smgrnblocks(reln, forknum); +} + /* * smgrtruncate() -- Truncate the given forks of supplied relation to * each specified numbers of blocks diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h index e15b20a566a..5f3fa3962a6 100644 --- a/src/include/storage/smgr.h +++ b/src/include/storage/smgr.h @@ -103,6 +103,7 @@ extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, BlockNumber nblocks); extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum); extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum); +extern void smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum); extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nblocks); extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum); -- 2.46.0