From 068023f37053e535d28191a0d52b47e1d360a261 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Tue, 15 Mar 2022 12:29:14 -0400 Subject: [PATCH v5] Fix possible recovery trouble if TRUNCATE overlaps a checkpoint. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If TRUNCATE causes some buffers to be invalidated and thus the checkpoint does not flush them, TRUNCATE must also ensure that the corresponding files are truncated on disk. Otherwise, a replay from the checkpoint might find that the buffers exist but have the wrong contents, which may cause replay to fail. Report by Teja Mupparti. Patch by Kyotaro Horiguchi, per a design suggestion from Heikki Linnakangas, with some changes to the comments by me. Review of this and a prior patch that approached the issue differently by Heikki Linnakangas, Andres Freund, Álvaro Herrera, Masahiko Sawada, and Tom Lane. Discussion: http://postgr.es/m/BYAPR06MB6373BF50B469CA393C614257ABF00@BYAPR06MB6373.namprd06.prod.outlook.com --- src/backend/access/transam/multixact.c | 6 ++-- src/backend/access/transam/twophase.c | 12 ++++---- src/backend/access/transam/xact.c | 5 ++-- src/backend/access/transam/xlog.c | 16 +++++++++-- src/backend/access/transam/xloginsert.c | 2 +- src/backend/catalog/storage.c | 29 ++++++++++++++++++- src/backend/storage/buffer/bufmgr.c | 6 ++-- src/backend/storage/ipc/procarray.c | 26 ++++++++++++----- src/backend/storage/lmgr/proc.c | 4 +-- src/include/storage/proc.h | 37 ++++++++++++++++++++++++- src/include/storage/procarray.h | 5 ++-- 11 files changed, 120 insertions(+), 28 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 6a70d49738..9f65c600d0 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -3088,8 +3088,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB) * crash/basebackup, even though the state of the data directory would * require it. */ - Assert(!MyProc->delayChkpt); - MyProc->delayChkpt = true; + Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0); + MyProc->delayChkpt |= DELAY_CHKPT_START; /* WAL log truncation */ WriteMTruncateXlogRec(newOldestMultiDB, @@ -3115,7 +3115,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB) /* Then offsets */ PerformOffsetsTruncation(oldestMulti, newOldestMulti); - MyProc->delayChkpt = false; + MyProc->delayChkpt &= ~DELAY_CHKPT_START; END_CRIT_SECTION(); LWLockRelease(MultiXactTruncationLock); diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 874c8ed125..4dc8ccc12b 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -475,7 +475,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid, } proc->xid = xid; Assert(proc->xmin == InvalidTransactionId); - proc->delayChkpt = false; + proc->delayChkpt = 0; proc->statusFlags = 0; proc->pid = 0; proc->databaseId = databaseid; @@ -1164,7 +1164,8 @@ EndPrepare(GlobalTransaction gxact) START_CRIT_SECTION(); - MyProc->delayChkpt = true; + Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0); + MyProc->delayChkpt |= DELAY_CHKPT_START; XLogBeginInsert(); for (record = records.head; record != NULL; record = record->next) @@ -1207,7 +1208,7 @@ EndPrepare(GlobalTransaction gxact) * checkpoint starting after this will certainly see the gxact as a * candidate for fsyncing. */ - MyProc->delayChkpt = false; + MyProc->delayChkpt &= ~DELAY_CHKPT_START; /* * Remember that we have this GlobalTransaction entry locked for us. If @@ -2266,7 +2267,8 @@ RecordTransactionCommitPrepared(TransactionId xid, START_CRIT_SECTION(); /* See notes in RecordTransactionCommit */ - MyProc->delayChkpt = true; + Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0); + MyProc->delayChkpt |= DELAY_CHKPT_START; /* * Emit the XLOG commit record. Note that we mark 2PC commits as @@ -2314,7 +2316,7 @@ RecordTransactionCommitPrepared(TransactionId xid, TransactionIdCommitTree(xid, nchildren, children); /* Checkpoint can proceed now */ - MyProc->delayChkpt = false; + MyProc->delayChkpt &= ~DELAY_CHKPT_START; END_CRIT_SECTION(); diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 8964ddf3eb..3596a7d734 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1387,8 +1387,9 @@ RecordTransactionCommit(void) * This makes checkpoint's determination of which xacts are delayChkpt * a bit fuzzy, but it doesn't matter. */ + Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0); START_CRIT_SECTION(); - MyProc->delayChkpt = true; + MyProc->delayChkpt |= DELAY_CHKPT_START; SetCurrentTransactionStopTimestamp(); @@ -1489,7 +1490,7 @@ RecordTransactionCommit(void) */ if (markXidCommitted) { - MyProc->delayChkpt = false; + MyProc->delayChkpt &= ~DELAY_CHKPT_START; END_CRIT_SECTION(); } diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index ed16f279b1..2bc2b7b8b4 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6502,18 +6502,30 @@ CreateCheckPoint(int flags) * and we will correctly flush the update below. So we cannot miss any * xacts we need to wait for. */ - vxids = GetVirtualXIDsDelayingChkpt(&nvxids); + vxids = GetVirtualXIDsDelayingChkpt(&nvxids, DELAY_CHKPT_START); if (nvxids > 0) { do { pg_usleep(10000L); /* wait for 10 msec */ - } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids)); + } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids, + DELAY_CHKPT_START)); } pfree(vxids); CheckPointGuts(checkPoint.redo, flags); + vxids = GetVirtualXIDsDelayingChkpt(&nvxids, DELAY_CHKPT_COMPLETE); + if (nvxids > 0) + { + do + { + pg_usleep(10000L); /* wait for 10 msec */ + } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids, + DELAY_CHKPT_COMPLETE)); + } + pfree(vxids); + /* * Take a snapshot of running transactions and write this to WAL. This * allows us to reconstruct the state of running transactions during diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index f4eb54b63c..462e23503e 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -1011,7 +1011,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std) /* * Ensure no checkpoint can change our view of RedoRecPtr. */ - Assert(MyProc->delayChkpt); + Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0); /* * Update RedoRecPtr so that we can make the right decision diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c index 9b8075536a..ef06ec1e67 100644 --- a/src/backend/catalog/storage.c +++ b/src/backend/catalog/storage.c @@ -325,6 +325,22 @@ RelationTruncate(Relation rel, BlockNumber nblocks) RelationPreTruncate(rel); + /* + * Make sure that a concurrent checkpoint can't complete while truncation + * is in progress. + * + * The truncation operation might drop buffers that the checkpoint + * otherwise would have flushed. If it does, then it's essential that + * the files actually get truncated on disk before the checkpoint record + * is written. Otherwise, if reply begins from that checkpoint, the + * to-be-truncated buffers might still exist on disk but have older + * contents than expected, which can cause replay to fail. It's OK for + * the buffers to not exist on disk at all, but not for them to have the + * wrong contents. + */ + Assert((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0); + MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE; + /* * We WAL-log the truncation before actually truncating, which means * trouble if the truncation fails. If we then crash, the WAL replay @@ -363,13 +379,24 @@ RelationTruncate(Relation rel, BlockNumber nblocks) XLogFlush(lsn); } - /* Do the real work to truncate relation forks */ + /* + * This will first remove any buffers from the buffer pool that should no + * longer exist after truncation is complete, and then truncate the + * corresponding files on disk. + */ smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks); + /* We've done all the critical work, so checkpoints are OK now. */ + MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE; + /* * Update upper-level FSM pages to account for the truncation. This is * important because the just-truncated pages were likely marked as * all-free, and would be preferentially selected. + * + * NB: There's no point in delaying checkpoints until this is done. + * Because the FSM is not WAL-logged, we have to be prepared for the + * possibility of corruption after a crash anyway. */ if (need_fsm_vacuum) FreeSpaceMapVacuumRange(rel, nblocks, InvalidBlockNumber); diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index f5459c68f8..11005edc73 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -3911,7 +3911,9 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std) * essential that CreateCheckPoint waits for virtual transactions * rather than full transactionids. */ - MyProc->delayChkpt = delayChkpt = true; + Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0); + MyProc->delayChkpt |= DELAY_CHKPT_START; + delayChkpt = true; lsn = XLogSaveBufferForHint(buffer, buffer_std); } @@ -3944,7 +3946,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std) UnlockBufHdr(bufHdr, buf_state); if (delayChkpt) - MyProc->delayChkpt = false; + MyProc->delayChkpt &= ~DELAY_CHKPT_START; if (dirtied) { diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 13d192ec2b..735763cc24 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -698,7 +698,10 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid) proc->lxid = InvalidLocalTransactionId; proc->xmin = InvalidTransactionId; - proc->delayChkpt = false; /* be sure this is cleared in abort */ + + /* be sure this is cleared in abort */ + proc->delayChkpt = 0; + proc->recoveryConflictPending = false; /* must be cleared with xid/xmin: */ @@ -737,7 +740,10 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid) proc->xid = InvalidTransactionId; proc->lxid = InvalidLocalTransactionId; proc->xmin = InvalidTransactionId; - proc->delayChkpt = false; /* be sure this is cleared in abort */ + + /* be sure this is cleared in abort */ + proc->delayChkpt = 0; + proc->recoveryConflictPending = false; /* must be cleared with xid/xmin: */ @@ -3053,7 +3059,8 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly) * delaying checkpoint because they have critical actions in progress. * * Constructs an array of VXIDs of transactions that are currently in commit - * critical sections, as shown by having delayChkpt set in their PGPROC. + * critical sections, as shown by having specified delayChkpt bits set in their + * PGPROC. * * Returns a palloc'd array that should be freed by the caller. * *nvxids is the number of valid entries. @@ -3067,13 +3074,15 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly) * for clearing of delayChkpt to propagate is unimportant for correctness. */ VirtualTransactionId * -GetVirtualXIDsDelayingChkpt(int *nvxids) +GetVirtualXIDsDelayingChkpt(int *nvxids, int type) { VirtualTransactionId *vxids; ProcArrayStruct *arrayP = procArray; int count = 0; int index; + Assert(type != 0); + /* allocate what's certainly enough result space */ vxids = (VirtualTransactionId *) palloc(sizeof(VirtualTransactionId) * arrayP->maxProcs); @@ -3085,7 +3094,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids) int pgprocno = arrayP->pgprocnos[index]; PGPROC *proc = &allProcs[pgprocno]; - if (proc->delayChkpt) + if ((proc->delayChkpt & type) != 0) { VirtualTransactionId vxid; @@ -3111,12 +3120,14 @@ GetVirtualXIDsDelayingChkpt(int *nvxids) * those numbers should be small enough for it not to be a problem. */ bool -HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) +HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type) { bool result = false; ProcArrayStruct *arrayP = procArray; int index; + Assert(type != 0); + LWLockAcquire(ProcArrayLock, LW_SHARED); for (index = 0; index < arrayP->numProcs; index++) @@ -3127,7 +3138,8 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids) GET_VXID_FROM_PGPROC(vxid, *proc); - if (proc->delayChkpt && VirtualTransactionIdIsValid(vxid)) + if ((proc->delayChkpt & type) != 0 && + VirtualTransactionIdIsValid(vxid)) { int i; diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 90283f8a9f..df080cd332 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -393,7 +393,7 @@ InitProcess(void) MyProc->roleId = InvalidOid; MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; - MyProc->delayChkpt = false; + MyProc->delayChkpt = 0; MyProc->statusFlags = 0; /* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */ if (IsAutoVacuumWorkerProcess()) @@ -578,7 +578,7 @@ InitAuxiliaryProcess(void) MyProc->roleId = InvalidOid; MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; - MyProc->delayChkpt = false; + MyProc->delayChkpt = 0; MyProc->statusFlags = 0; MyProc->lwWaiting = false; MyProc->lwWaitMode = 0; diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index a58888f9e9..36ecf7d005 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -86,6 +86,41 @@ struct XidCache */ #define INVALID_PGPROCNO PG_INT32_MAX +/* + * Flags for PGPROC.delayChkpt + * + * These flags can be used to delay the start or completion of a checkpoint + * for short periods. A flag is in effect if the corresponding bit is set in + * the PGPROC of any backend. + * + * For our purposes here, a checkpoint has three phases: (1) determine the + * location to which the redo pointer will be moved, (2) write all the + * data durably to disk, and (3) WAL-log the checkpoint. + * + * Setting DELAY_CHKPT_START prevents the system from moving from phase 1 + * to phase 2. This is useful when we are performing a WAL-logged modification + * of data that will be flushed to disk in phase 2. By setting this flag + * before writing WAL and clearing it after we've both written WAL and + * performed the corresponding modification, we ensure that if the WAL record + * is inserted prior to the new redo point, the corresponding data changes will + * also be flushed to disk before the checkpoint can complete. (In the + * extremely common case where the data being modified is in shared buffers + * and we acquire an exclusive content lock on the relevant buffers before + * writing WAL, this mechanism is not needed, because phase 2 will block + * until we release the content lock and then flush the modified data to + * disk.) + * + * Setting DELAY_CHKPT_COMPLETE prevents the system from moving from phase 2 + * to phase 3. This is useful if we are performing a WAL-logged operation that + * might invalidate buffers, such as relation truncation. In this case, we need + * to ensure that any buffers which were invalidated and thus not flushed by + * the checkpoint are actaully destroyed on disk. Replay can cope with a file + * or block that doesn't exist, but not with a block that has the wrong + * contents. + */ +#define DELAY_CHKPT_START (1<<0) +#define DELAY_CHKPT_COMPLETE (1<<1) + typedef enum { PROC_WAIT_STATUS_OK, @@ -191,7 +226,7 @@ struct PGPROC pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition * started */ - bool delayChkpt; /* true if this proc delays checkpoint start */ + int delayChkpt; /* for DELAY_CHKPT_* flags */ uint8 statusFlags; /* this backend's status flags, see PROC_* * above. mirrored in diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h index e03692053e..1b2cfac5ad 100644 --- a/src/include/storage/procarray.h +++ b/src/include/storage/procarray.h @@ -59,8 +59,9 @@ extern TransactionId GetOldestActiveTransactionId(void); extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly); extern void GetReplicationHorizons(TransactionId *slot_xmin, TransactionId *catalog_xmin); -extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids); -extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids); +extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type); +extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, + int nvxids, int type); extern PGPROC *BackendPidGetProc(int pid); extern PGPROC *BackendPidGetProcWithLock(int pid); -- 2.24.3 (Apple Git-128)