From a4ddbbd1a4d277e2e6d65abce320b6a1dd944c14 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 7 Jan 2001 22:14:31 +0000
Subject: [PATCH] Correct nasty error in heap_update: it was releasing the
 buffer refcount before calling RelationInvalidateHeapTuple(), which is bad
 because the latter needs to look at the tuple data, which is in the shared
 disk buffer.  If another backend manages to recycle the buffer while this is
 going on, we will compute the wrong hashindex for the tuple or maybe even
 crash outright.  Must hold buffer refcount until afterwards. (This bug is not
 in 7.0.*; seems to be have introduced during WAL changes.)

---
 src/backend/access/heap/heapam.c | 48 +++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ffeb101096..612e4c6861 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.105 2000/12/30 15:19:54 vadim Exp $
+ *	  $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.106 2001/01/07 22:14:31 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1410,8 +1410,13 @@ heap_insert(Relation relation, HeapTuple tup)
 	LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 	WriteBuffer(buffer);
 
-	if (IsSystemRelationName(RelationGetRelationName(relation)))
-		RelationMark4RollbackHeapTuple(relation, tup);
+	/*
+	 * If tuple is cachable, mark it for rollback from the caches
+	 * in case we abort.  Note it is OK to do this after WriteBuffer
+	 * releases the buffer, because the "tup" data structure is all
+	 * in local memory, not in the shared buffer.
+	 */
+	RelationMark4RollbackHeapTuple(relation, tup);
 
 	return tup->t_data->t_oid;
 }
@@ -1541,7 +1546,11 @@ l1:
 
 	LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
-	/* invalidate caches */
+	/*
+	 * Mark tuple for invalidation from system caches at next command boundary.
+	 * We have to do this before WriteBuffer because we need to look at the
+	 * contents of the tuple, so we need to hold our refcount on the buffer.
+	 */
 	RelationInvalidateHeapTuple(relation, &tp);
 
 	WriteBuffer(buffer);
@@ -1570,7 +1579,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
 
 	buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(otid));
 	if (!BufferIsValid(buffer))
-		elog(ERROR, "amreplace: failed ReadBuffer");
+		elog(ERROR, "heap_update: failed ReadBuffer");
 	LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 
 	dp = (PageHeader) BufferGetPage(buffer);
@@ -1580,6 +1589,12 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
 	oldtup.t_data = (HeapTupleHeader) PageGetItem(dp, lp);
 	oldtup.t_len = ItemIdGetLength(lp);
 	oldtup.t_self = *otid;
+	/*
+	 * Note: beyond this point, use oldtup not otid to refer to old tuple.
+	 * otid may very well point at newtup->t_self, which we will overwrite
+	 * with the new tuple's location, so there's great risk of confusion
+	 * if we use otid anymore.
+	 */
 
 l2:
 	result = HeapTupleSatisfiesUpdate(&oldtup);
@@ -1672,7 +1687,7 @@ l2:
 		 * after postmaster startup.
 		 */
 		_locked_tuple_.node = relation->rd_node;
-		_locked_tuple_.tid = *otid;
+		_locked_tuple_.tid = oldtup.t_self;
 		XactPushRollback(_heap_unlock_tuple, (void*) &_locked_tuple_);
 
 		TransactionIdStore(GetCurrentTransactionId(), &(oldtup.t_data->t_xmax));
@@ -1722,15 +1737,26 @@ l2:
 	END_CRIT_CODE;
 
 	if (newbuf != buffer)
-	{
 		LockBuffer(newbuf, BUFFER_LOCK_UNLOCK);
-		WriteBuffer(newbuf);
-	}
 	LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-	WriteBuffer(buffer);
 
-	/* invalidate caches */
+	/*
+	 * Mark old tuple for invalidation from system caches at next command
+	 * boundary. We have to do this before WriteBuffer because we need to
+	 * look at the contents of the tuple, so we need to hold our refcount.
+	 */
 	RelationInvalidateHeapTuple(relation, &oldtup);
+
+	if (newbuf != buffer)
+		WriteBuffer(newbuf);
+	WriteBuffer(buffer);
+
+	/*
+	 * If new tuple is cachable, mark it for rollback from the caches
+	 * in case we abort.  Note it is OK to do this after WriteBuffer
+	 * releases the buffer, because the "newtup" data structure is all
+	 * in local memory, not in the shared buffer.
+	 */
 	RelationMark4RollbackHeapTuple(relation, newtup);
 
 	return HeapTupleMayBeUpdated;
-- 
2.11.0