postgres.git / summary / log / commit / refs
commit d3e5d89504484d69bc41e66fc2c2346ec0da6b07
Author: Noah Misch <noah@leadboat.com>
Date: Wed Dec 17 00:13:54 2025 +0000
First released in: 14.21, 15.16, 16.12, 17.8
WAL-log inplace update before revealing it to other sessions.
A buffer lock won't stop a reader having already checked tuple
visibility. If a vac_update_datfrozenid() and then a crash happened
during inplace update of a relfrozenxid value, datfrozenxid could
overtake relfrozenxid. That could lead to "could not access status of
transaction" errors.
Back-patch to v14 - v17. This is a back-patch of commits:
- 8e7e672cdaa6bfec85d4d5dd9be84159df23bb41
(main change, on master, before v18 branched)
- 818013665259d4242ba641aad705ebe5a3e2db8e
(defect fix, on master, before v18 branched)
It reverses commit bc6bad88572501aecaa2ac5d4bc900ac0fd457d5, my revert
of the original back-patch.
In v14, this also back-patches the assertion removal from commit
7fcf2faf9c7dd473208fd6d5565f88d7f733782b.
Discussion: https://postgr.es/m/20240620012908.92.nmisch@google.com
Backpatch-through: 14-17
src/backend/access/heap/README.tuplock | 4 +-
src/backend/access/heap/heapam.c | 68 +++++++++++++++++++++++++++-------
src/include/storage/proc.h | 8 ++--
3 files changed, 59 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index ad835ff4820..16f7d78b7d2 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -198,9 +198,7 @@ Inplace updates create an exception to the rule that tuple data won't change
under a reader holding a pin. A reader of a heap_fetch() result tuple may
witness a torn read. Current inplace-updated fields are aligned and are no
wider than four bytes, and current readers don't need consistency across
-fields. Hence, they get by with just fetching each field once. XXX such a
-caller may also read a value that has not reached WAL; see
-systable_inplace_update_finish().
+fields. Hence, they get by with just fetching each field once.
During logical decoding, caches reflect an inplace update no later than the
next XLOG_XACT_INVALIDATIONS. That record witnesses the end of a command.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 731d504489b..ce26d21efe2 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6450,6 +6450,8 @@ heap_inplace_update_and_unlock(Relation relation,
HeapTupleHeader htup = oldtup->t_data;
uint32 oldlen;
uint32 newlen;
+ char *dst;
+ char *src;
Assert(ItemPointerEquals(&oldtup->t_self, &tuple->t_self));
oldlen = oldtup->t_len - htup->t_hoff;
@@ -6457,6 +6459,9 @@ heap_inplace_update_and_unlock(Relation relation,
if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff)
elog(ERROR, "wrong tuple length");
+ dst = (char *) htup + htup->t_hoff;
+ src = (char *) tuple->t_data + tuple->t_data->t_hoff;
+
/*
* Unlink relcache init files as needed. If unlinking, acquire
* RelCacheInitLock until after associated invalidations. By doing this
@@ -6467,15 +6472,15 @@ heap_inplace_update_and_unlock(Relation relation,
*/
PreInplace_Inval();
- /* NO EREPORT(ERROR) from here till changes are logged */
- START_CRIT_SECTION();
-
- memcpy((char *) htup + htup->t_hoff,
- (char *) tuple->t_data + tuple->t_data->t_hoff,
- newlen);
-
/*----------
- * XXX A crash here can allow datfrozenxid() to get ahead of relfrozenxid:
+ * NO EREPORT(ERROR) from here till changes are complete
+ *
+ * Our buffer lock won't stop a reader having already pinned and checked
+ * visibility for this tuple. Hence, we write WAL first, then mutate the
+ * buffer. Like in MarkBufferDirtyHint() or RecordTransactionCommit(),
+ * checkpoint delay makes that acceptable. With the usual order of
+ * changes, a crash after memcpy() and before XLogInsert() could allow
+ * datfrozenxid to overtake relfrozenxid:
*
* ["D" is a VACUUM (ONLY_DATABASE_STATS)]
* ["R" is a VACUUM tbl]
@@ -6485,14 +6490,36 @@ heap_inplace_update_and_unlock(Relation relation,
* D: raise pg_database.datfrozenxid, XLogInsert(), finish
* [crash]
* [recovery restores datfrozenxid w/o relfrozenxid]
- */
-
- MarkBufferDirty(buffer);
+ *
+ * Mimic MarkBufferDirtyHint() subroutine XLogSaveBufferForHint().
+ * Specifically, use DELAY_CHKPT_START, and copy the buffer to the stack.
+ * The stack copy facilitates a FPI of the post-mutation block before we
+ * accept other sessions seeing it. DELAY_CHKPT_START allows us to
+ * XLogInsert() before MarkBufferDirty(). Since XLogSaveBufferForHint()
+ * can operate under BUFFER_LOCK_SHARED, it can't avoid DELAY_CHKPT_START.
+ * This function, however, likely could avoid it with the following order
+ * of operations: MarkBufferDirty(), XLogInsert(), memcpy(). Opt to use
+ * DELAY_CHKPT_START here, too, as a way to have fewer distinct code
+ * patterns to analyze. Inplace update isn't so frequent that it should
+ * pursue the small optimization of skipping DELAY_CHKPT_START.
+ */
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
+ START_CRIT_SECTION();
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START;
/* XLOG stuff */
if (RelationNeedsWAL(relation))
{
xl_heap_inplace xlrec;
+ PGAlignedBlock copied_buffer;
+ char *origdata = (char *) BufferGetBlock(buffer);
+ Page page = BufferGetPage(buffer);
+ uint16 lower = ((PageHeader) page)->pd_lower;
+ uint16 upper = ((PageHeader) page)->pd_upper;
+ uintptr_t dst_offset_in_block;
+ RelFileLocator rlocator;
+ ForkNumber forkno;
+ BlockNumber blkno;
XLogRecPtr recptr;
xlrec.offnum = ItemPointerGetOffsetNumber(&tuple->t_self);
@@ -6500,16 +6527,28 @@ heap_inplace_update_and_unlock(Relation relation,
XLogBeginInsert();
XLogRegisterData((char *) &xlrec, SizeOfHeapInplace);
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
- XLogRegisterBufData(0, (char *) htup + htup->t_hoff, newlen);
+ /* register block matching what buffer will look like after changes */
+ memcpy(copied_buffer.data, origdata, lower);
+ memcpy(copied_buffer.data + upper, origdata + upper, BLCKSZ - upper);
+ dst_offset_in_block = dst - origdata;
+ memcpy(copied_buffer.data + dst_offset_in_block, src, newlen);
+ BufferGetTag(buffer, &rlocator, &forkno, &blkno);
+ Assert(forkno == MAIN_FORKNUM);
+ XLogRegisterBlock(0, &rlocator, forkno, blkno, copied_buffer.data,
+ REGBUF_STANDARD);
+ XLogRegisterBufData(0, src, newlen);
/* inplace updates aren't decoded atm, don't log the origin */
recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_INPLACE);
- PageSetLSN(BufferGetPage(buffer), recptr);
+ PageSetLSN(page, recptr);
}
+ memcpy(dst, src, newlen);
+
+ MarkBufferDirty(buffer);
+
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
/*
@@ -6518,6 +6557,7 @@ heap_inplace_update_and_unlock(Relation relation,
*/
AtInplace_Inval();
+ MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
END_CRIT_SECTION();
UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index bf0a714d60e..2af6a355b3a 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -104,10 +104,10 @@ struct XidCache
* 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.)
+ * and we acquire an exclusive content lock and MarkBufferDirty() 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. See transam/README and SyncOneBuffer().)
*
* 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
[parent: 0f69beddea11]