Modify tuptoaster's API so that it does not try to modify the passed

tuple in-place, but instead passes back an all-new tuple structure if
any changes are needed.  This is a much cleaner and more robust solution
for the bug discovered by Alexey Beschiokov; accordingly, revert the
quick hack I installed yesterday.
With this change, HeapTupleData.t_datamcxt is no longer needed; will
remove it in a separate commit in HEAD only.

4 files changed, 124 insertions, 101 deletions

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 6c669ed62b4..fc750884c75 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.200 2005/10/15 02:49:08 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.201 2005/11/20 18:38:20 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1085,12 +1085,19 @@ heap_get_latest_tid(Relation relation,
  *
  * use_fsm is passed directly to RelationGetBufferForTuple, which see for
  * more info.
+ *
+ * The return value is the OID assigned to the tuple (either here or by the
+ * caller), or InvalidOid if no OID.  The header fields of *tup are updated
+ * to match the stored tuple; in particular tup->t_self receives the actual
+ * TID where the tuple was stored.  But note that any toasting of fields
+ * within the tuple data is NOT reflected into *tup.
  */
 Oid
 heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 			bool use_wal, bool use_fsm)
 {
 	TransactionId xid = GetCurrentTransactionId();
+	HeapTuple	heaptup;
 	Buffer		buffer;
 
 	if (relation->rd_rel->relhasoids)
@@ -1128,19 +1135,24 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 	/*
 	 * If the new tuple is too big for storage or contains already toasted
 	 * out-of-line attributes from some other relation, invoke the toaster.
+	 *
+	 * Note: below this point, heaptup is the data we actually intend to
+	 * store into the relation; tup is the caller's original untoasted data.
 	 */
 	if (HeapTupleHasExternal(tup) ||
 		(MAXALIGN(tup->t_len) > TOAST_TUPLE_THRESHOLD))
-		heap_tuple_toast_attrs(relation, tup, NULL);
+		heaptup = toast_insert_or_update(relation, tup, NULL);
+	else
+		heaptup = tup;
 
 	/* Find buffer to insert this tuple into */
-	buffer = RelationGetBufferForTuple(relation, tup->t_len,
+	buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
 									   InvalidBuffer, use_fsm);
 
 	/* NO EREPORT(ERROR) from here till changes are logged */
 	START_CRIT_SECTION();
 
-	RelationPutHeapTuple(relation, buffer, tup);
+	RelationPutHeapTuple(relation, buffer, heaptup);
 
 	/* XLOG stuff */
 	if (relation->rd_istemp)
@@ -1158,15 +1170,15 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 		uint8		info = XLOG_HEAP_INSERT;
 
 		xlrec.target.node = relation->rd_node;
-		xlrec.target.tid = tup->t_self;
+		xlrec.target.tid = heaptup->t_self;
 		rdata[0].data = (char *) &xlrec;
 		rdata[0].len = SizeOfHeapInsert;
 		rdata[0].buffer = InvalidBuffer;
 		rdata[0].next = &(rdata[1]);
 
-		xlhdr.t_natts = tup->t_data->t_natts;
-		xlhdr.t_infomask = tup->t_data->t_infomask;
-		xlhdr.t_hoff = tup->t_data->t_hoff;
+		xlhdr.t_natts = heaptup->t_data->t_natts;
+		xlhdr.t_infomask = heaptup->t_data->t_infomask;
+		xlhdr.t_hoff = heaptup->t_data->t_hoff;
 
 		/*
 		 * note we mark rdata[1] as belonging to buffer; if XLogInsert decides
@@ -1180,8 +1192,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 		rdata[1].next = &(rdata[2]);
 
 		/* PG73FORMAT: write bitmap [+ padding] [+ oid] + data */
-		rdata[2].data = (char *) tup->t_data + offsetof(HeapTupleHeaderData, t_bits);
-		rdata[2].len = tup->t_len - offsetof(HeapTupleHeaderData, t_bits);
+		rdata[2].data = (char *) heaptup->t_data + offsetof(HeapTupleHeaderData, t_bits);
+		rdata[2].len = heaptup->t_len - offsetof(HeapTupleHeaderData, t_bits);
 		rdata[2].buffer = buffer;
 		rdata[2].buffer_std = true;
 		rdata[2].next = NULL;
@@ -1191,7 +1203,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 		 * page instead of restoring the whole thing.  Set flag, and hide
 		 * buffer references from XLogInsert.
 		 */
-		if (ItemPointerGetOffsetNumber(&(tup->t_self)) == FirstOffsetNumber &&
+		if (ItemPointerGetOffsetNumber(&(heaptup->t_self)) == FirstOffsetNumber &&
 			PageGetMaxOffsetNumber(page) == FirstOffsetNumber)
 		{
 			info |= XLOG_HEAP_INIT_PAGE;
@@ -1212,13 +1224,23 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 	/*
 	 * If tuple is cachable, mark it for invalidation 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.
+	 * buffer, because the heaptup data structure is all in local memory,
+	 * not in the shared buffer.
 	 */
-	CacheInvalidateHeapTuple(relation, tup);
+	CacheInvalidateHeapTuple(relation, heaptup);
 
 	pgstat_count_heap_insert(&relation->pgstat_info);
 
+	/*
+	 * If heaptup is a private copy, release it.  Don't forget to copy t_self
+	 * back to the caller's image, too.
+	 */
+	if (heaptup != tup)
+	{
+		tup->t_self = heaptup->t_self;
+		heap_freetuple(heaptup);
+	}
+
 	return HeapTupleGetOid(tup);
 }
 
@@ -1469,7 +1491,7 @@ l1:
 	 * context lock on the buffer first.
 	 */
 	if (HeapTupleHasExternal(&tp))
-		heap_tuple_toast_attrs(relation, NULL, &tp);
+		toast_delete(relation, &tp);
 
 	/*
 	 * Mark tuple for invalidation from system caches at next command
@@ -1553,8 +1575,10 @@ simple_heap_delete(Relation relation, ItemPointer tid)
  * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
  * (the last only possible if wait == false).
  *
- * On success, newtup->t_self is set to the TID where the new tuple
- * was inserted.
+ * On success, the header fields of *newtup are updated to match the new
+ * stored tuple; in particular, newtup->t_self is set to the TID where the
+ * new tuple was inserted.  However, any TOAST changes in the new tuple's
+ * data are not reflected into *newtup.
  *
  * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
  * If t_ctid is the same as otid, the tuple was deleted; if different, the
@@ -1570,6 +1594,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
 	TransactionId xid = GetCurrentTransactionId();
 	ItemId		lp;
 	HeapTupleData oldtup;
+	HeapTuple	heaptup;
 	PageHeader	dp;
 	Buffer		buffer,
 				newbuf;
@@ -1760,11 +1785,12 @@ l2:
 	 * We need to invoke the toaster if there are already any out-of-line toasted
 	 * values present, or if the new tuple is over-threshold.
 	 */
+	newtupsize = MAXALIGN(newtup->t_len);
+
 	need_toast = (HeapTupleHasExternal(&oldtup) ||
 				  HeapTupleHasExternal(newtup) ||
-				  (MAXALIGN(newtup->t_len) > TOAST_TUPLE_THRESHOLD));
+				  newtupsize > TOAST_TUPLE_THRESHOLD);
 
-	newtupsize = MAXALIGN(newtup->t_len);
 	pagefree = PageGetFreeSpace((Page) dp);
 
 	if (need_toast || newtupsize > pagefree)
@@ -1776,15 +1802,25 @@ l2:
 									   HEAP_MOVED);
 		HeapTupleHeaderSetXmax(oldtup.t_data, xid);
 		HeapTupleHeaderSetCmax(oldtup.t_data, cid);
+		/* temporarily make it look not-updated */
+		oldtup.t_data->t_ctid = oldtup.t_self;
 		already_marked = true;
 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
-		/* Let the toaster do its thing */
+		/*
+		 * Let the toaster do its thing, if needed.
+		 *
+		 * Note: below this point, heaptup is the data we actually intend to
+		 * store into the relation; newtup is the caller's original untoasted
+		 * data.
+		 */
 		if (need_toast)
 		{
-			heap_tuple_toast_attrs(relation, newtup, &oldtup);
-			newtupsize = MAXALIGN(newtup->t_len);
+			heaptup = toast_insert_or_update(relation, newtup, &oldtup);
+			newtupsize = MAXALIGN(heaptup->t_len);
 		}
+		else
+			heaptup = newtup;
 
 		/*
 		 * Now, do we need a new page for the tuple, or not?  This is a bit
@@ -1805,8 +1841,8 @@ l2:
 		 */
 		if (newtupsize > pagefree)
 		{
-			/* Assume there's no chance to put newtup on same page. */
-			newbuf = RelationGetBufferForTuple(relation, newtup->t_len,
+			/* Assume there's no chance to put heaptup on same page. */
+			newbuf = RelationGetBufferForTuple(relation, heaptup->t_len,
 											   buffer, true);
 		}
 		else
@@ -1823,7 +1859,7 @@ l2:
 				 * seldom be taken.
 				 */
 				LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-				newbuf = RelationGetBufferForTuple(relation, newtup->t_len,
+				newbuf = RelationGetBufferForTuple(relation, heaptup->t_len,
 												   buffer, true);
 			}
 			else
@@ -1838,6 +1874,7 @@ l2:
 		/* No TOAST work needed, and it'll fit on same page */
 		already_marked = false;
 		newbuf = buffer;
+		heaptup = newtup;
 	}
 
 	/*
@@ -1849,7 +1886,7 @@ l2:
 	/* NO EREPORT(ERROR) from here till changes are logged */
 	START_CRIT_SECTION();
 
-	RelationPutHeapTuple(relation, newbuf, newtup);		/* insert new tuple */
+	RelationPutHeapTuple(relation, newbuf, heaptup); /* insert new tuple */
 
 	if (!already_marked)
 	{
@@ -1863,13 +1900,13 @@ l2:
 	}
 
 	/* record address of new tuple in t_ctid of old one */
-	oldtup.t_data->t_ctid = newtup->t_self;
+	oldtup.t_data->t_ctid = heaptup->t_self;
 
 	/* XLOG stuff */
 	if (!relation->rd_istemp)
 	{
 		XLogRecPtr	recptr = log_heap_update(relation, buffer, oldtup.t_self,
-											 newbuf, newtup, false);
+											 newbuf, heaptup, false);
 
 		if (newbuf != buffer)
 		{
@@ -1905,10 +1942,10 @@ l2:
 	/*
 	 * If new tuple is cachable, mark it for invalidation 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
+	 * buffer, because the heaptup data structure is all in local memory, not
 	 * in the shared buffer.
 	 */
-	CacheInvalidateHeapTuple(relation, newtup);
+	CacheInvalidateHeapTuple(relation, heaptup);
 
 	/*
 	 * Release the lmgr tuple lock, if we had it.
@@ -1918,6 +1955,16 @@ l2:
 
 	pgstat_count_heap_update(&relation->pgstat_info);
 
+	/*
+	 * If heaptup is a private copy, release it.  Don't forget to copy t_self
+	 * back to the caller's image, too.
+	 */
+	if (heaptup != newtup)
+	{
+		newtup->t_self = heaptup->t_self;
+		heap_freetuple(heaptup);
+	}
+
 	return HeapTupleMayBeUpdated;
 }
 
diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index fd20f111b80..99d725c27cc 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -8,14 +8,17 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/access/heap/tuptoaster.c,v 1.53 2005/10/15 02:49:09 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/access/heap/tuptoaster.c,v 1.54 2005/11/20 18:38:20 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
- *		heap_tuple_toast_attrs -
+ *		toast_insert_or_update -
  *			Try to make a given tuple fit into one page by compressing
  *			or moving off attributes
  *
+ *		toast_delete -
+ *			Reclaim toast storage when a tuple is deleted
+ *
  *		heap_tuple_untoast_attr -
  *			Fetch back a given value from the "secondary" relation
  *
@@ -40,10 +43,7 @@
 
 #undef TOAST_DEBUG
 
-static void toast_delete(Relation rel, HeapTuple oldtup);
 static void toast_delete_datum(Relation rel, Datum value);
-static void toast_insert_or_update(Relation rel, HeapTuple newtup,
-					   HeapTuple oldtup);
 static Datum toast_save_datum(Relation rel, Datum value);
 static varattrib *toast_fetch_datum(varattrib *attr);
 static varattrib *toast_fetch_datum_slice(varattrib *attr,
@@ -51,24 +51,6 @@ static varattrib *toast_fetch_datum_slice(varattrib *attr,
 
 
 /* ----------
- * heap_tuple_toast_attrs -
- *
- *	This is the central public entry point for toasting from heapam.
- *
- *	Calls the appropriate event specific action.
- * ----------
- */
-void
-heap_tuple_toast_attrs(Relation rel, HeapTuple newtup, HeapTuple oldtup)
-{
-	if (newtup == NULL)
-		toast_delete(rel, oldtup);
-	else
-		toast_insert_or_update(rel, newtup, oldtup);
-}
-
-
-/* ----------
  * heap_tuple_fetch_attr -
  *
  *	Public entry point to get back a toasted value
@@ -305,7 +287,7 @@ toast_datum_size(Datum value)
  *	Cascaded delete toast-entries on DELETE
  * ----------
  */
-static void
+void
 toast_delete(Relation rel, HeapTuple oldtup)
 {
 	TupleDesc	tupleDesc;
@@ -355,11 +337,22 @@ toast_delete(Relation rel, HeapTuple oldtup)
  *
  *	Delete no-longer-used toast-entries and create new ones to
  *	make the new tuple fit on INSERT or UPDATE
+ *
+ * Inputs:
+ *	newtup: the candidate new tuple to be inserted
+ *	oldtup: the old row version for UPDATE, or NULL for INSERT
+ * Result:
+ *	either newtup if no toasting is needed, or a palloc'd modified tuple
+ *	that is what should actually get stored
+ *
+ * NOTE: neither newtup nor oldtup will be modified.  This is a change
+ * from the pre-8.1 API of this routine.
  * ----------
  */
-static void
+HeapTuple
 toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 {
+	HeapTuple	result_tuple;
 	TupleDesc	tupleDesc;
 	Form_pg_attribute *att;
 	int			numAttrs;
@@ -757,7 +750,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 	if (need_change)
 	{
 		HeapTupleHeader olddata = newtup->t_data;
-		char	   *new_data;
+		HeapTupleHeader new_data;
 		int32		new_len;
 
 		/*
@@ -775,14 +768,18 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 										  toast_values, toast_isnull);
 
 		/*
-		 * Allocate new tuple in same context as old one.
+		 * Allocate and zero the space needed, and fill HeapTupleData fields.
 		 */
-		new_data = (char *) MemoryContextAlloc(newtup->t_datamcxt, new_len);
-		newtup->t_data = (HeapTupleHeader) new_data;
-		newtup->t_len = new_len;
+		result_tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + new_len);
+		result_tuple->t_len = new_len;
+		result_tuple->t_self = newtup->t_self;
+		result_tuple->t_tableOid = newtup->t_tableOid;
+		result_tuple->t_datamcxt = CurrentMemoryContext;
+		new_data = (HeapTupleHeader) ((char *) result_tuple + HEAPTUPLESIZE);
+		result_tuple->t_data = new_data;
 
 		/*
-		 * Put the tuple header and the changed values into place
+		 * Put the existing tuple header and the changed values into place
 		 */
 		memcpy(new_data, olddata, olddata->t_hoff);
 
@@ -790,16 +787,11 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 						toast_values,
 						toast_isnull,
 						(char *) new_data + olddata->t_hoff,
-						&(newtup->t_data->t_infomask),
-						has_nulls ? newtup->t_data->t_bits : NULL);
-
-		/*
-		 * In the case we modified a previously modified tuple again, free the
-		 * memory from the previous run
-		 */
-		if ((char *) olddata != ((char *) newtup + HEAPTUPLESIZE))
-			pfree(olddata);
+						&(new_data->t_infomask),
+						has_nulls ? new_data->t_bits : NULL);
 	}
+	else
+		result_tuple = newtup;
 
 	/*
 	 * Free allocated temp values
@@ -816,6 +808,8 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 		for (i = 0; i < numAttrs; i++)
 			if (toast_delold[i])
 				toast_delete_datum(rel, toast_oldvalues[i]);
+
+	return result_tuple;
 }
 
 
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 6f79c4d58b0..c0c6cfbe500 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -26,7 +26,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.259 2005/11/19 20:57:44 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.260 2005/11/20 18:38:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1446,16 +1446,6 @@ ExecInsert(TupleTableSlot *slot,
 	setLastTid(&(tuple->t_self));
 
 	/*
-	 * KLUGE SOLUTION for bug found post 8.1 release: if the tuple toaster
-	 * fired on the tuple then it changed the physical tuple inside the
-	 * tuple slot, leaving any extracted information invalid.  Mark the
-	 * extracted state invalid just in case.  Need to fix things so that
-	 * the toaster gets to run against the tuple before we materialize it,
-	 * but that's way too invasive for a stable branch.
-	 */
-	slot->tts_nvalid = 0;
-
-	/*
 	 * insert index entries for tuple
 	 */
 	if (resultRelInfo->ri_NumIndices > 0)
@@ -1710,16 +1700,6 @@ lreplace:;
 	(estate->es_processed)++;
 
 	/*
-	 * KLUGE SOLUTION for bug found post 8.1 release: if the tuple toaster
-	 * fired on the tuple then it changed the physical tuple inside the
-	 * tuple slot, leaving any extracted information invalid.  Mark the
-	 * extracted state invalid just in case.  Need to fix things so that
-	 * the toaster gets to run against the tuple before we materialize it,
-	 * but that's way too invasive for a stable branch.
-	 */
-	slot->tts_nvalid = 0;
-
-	/*
 	 * Note: instead of having to update the old index tuples associated with
 	 * the heap tuple, all we do is form and insert new index tuples. This is
 	 * because UPDATEs are actually DELETEs and INSERTs, and index tuple
diff --git a/src/include/access/tuptoaster.h b/src/include/access/tuptoaster.h
index b45df277b1e..b5870e69dcb 100644
--- a/src/include/access/tuptoaster.h
+++ b/src/include/access/tuptoaster.h
@@ -6,7 +6,7 @@
  *
  * Copyright (c) 2000-2005, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/include/access/tuptoaster.h,v 1.23 2005/07/06 19:02:53 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/tuptoaster.h,v 1.24 2005/11/20 18:38:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -66,19 +66,21 @@
 
 
 /* ----------
- * heap_tuple_toast_attrs() -
+ * toast_insert_or_update -
  *
- *		Called by heap_insert(), heap_update() and heap_delete().
- *		Outdates any no-longer-needed toast entries referenced
- *		by oldtup and creates new ones until newtup is no more than
- *		TOAST_TUPLE_TARGET (or we run out of toastable values).
- *		Possibly modifies newtup by replacing the t_data part!
+ *	Called by heap_insert() and heap_update().
+ * ----------
+ */
+extern HeapTuple toast_insert_or_update(Relation rel,
+										HeapTuple newtup, HeapTuple oldtup);
+
+/* ----------
+ * toast_delete -
  *
- *		oldtup is NULL if insert, newtup is NULL if delete.
+ *	Called by heap_delete().
  * ----------
  */
-extern void heap_tuple_toast_attrs(Relation rel,
-					   HeapTuple newtup, HeapTuple oldtup);
+extern void toast_delete(Relation rel, HeapTuple oldtup);
 
 /* ----------
  * heap_tuple_fetch_attr() -