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.)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ffeb101096f22d6f4fba5a230792fef8211f684f..612e4c68611fc52d87226715c5cba40e181e9790 100644 (file)
--- 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;