heapam: Don't mimic MarkBufferDirtyHint() in inplace updates

Previously heap_inplace_update_and_unlock() used an operation order similar to
MarkBufferDirty(), to reduce the number of different approaches used for
updating buffers.  However, in an upcoming patch, MarkBufferDirtyHint() will
switch to using the update protocol used by most other places (enabled by hint
bits only being set while holding a share-exclusive lock).

Luckily it's pretty easy to adjust heap_inplace_update_and_unlock(). As a
comment already foresaw, we can use the normal order, with the slight change
of updating the buffer contents after WAL logging.

Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/5ubipyssiju5twkb7zgqwdr7q2vhpkpmuelxfpanetlk6ofnop@hvxb4g2amb2d

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index a231563f0dfecd7f1d6f3f8086c9555363cb261c..1ecc83308510c9e389dec64b1171197f0818bb95 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6613,11 +6613,11 @@ heap_inplace_update_and_unlock(Relation relation,
        /*----------
         * 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
+        * Our exclusive buffer lock won't stop a reader having already pinned and
+        * checked visibility for this tuple. With the usual order of changes
+        * (i.e. updating the buffer contents before WAL logging), a reader could
+        * observe our not-yet-persistent update to relfrozenxid and update
+        * datfrozenxid based on that. A crash in that moment could allow
         * datfrozenxid to overtake relfrozenxid:
         *
         * ["D" is a VACUUM (ONLY_DATABASE_STATS)]
@@ -6629,21 +6629,15 @@ heap_inplace_update_and_unlock(Relation relation,
         * [crash]
         * [recovery restores datfrozenxid w/o relfrozenxid]
         *
-        * 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);
+        * We avoid that by using a temporary copy of the buffer to hide our
+        * change from other backends until the change has been WAL-logged. We
+        * apply our change to the temporary copy and WAL-log it, before modifying
+        * the real page. That way any action a reader of the in-place-updated
+        * value takes will be WAL logged after this change.
+        */
        START_CRIT_SECTION();
-       MyProc->delayChkptFlags |= DELAY_CHKPT_START;
+
+       MarkBufferDirty(buffer);
 
        /* XLOG stuff */
        if (RelationNeedsWAL(relation))
@@ -6692,8 +6686,6 @@ heap_inplace_update_and_unlock(Relation relation,
 
        memcpy(dst, src, newlen);
 
-       MarkBufferDirty(buffer);
-
        LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
        /*
@@ -6702,7 +6694,6 @@ heap_inplace_update_and_unlock(Relation relation,
         */
        AtInplace_Inval();
 
-       MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
        END_CRIT_SECTION();
        UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock);