Refactoring around ExecInsert.

This is mostly in preparation for changes soon to be merged from PostgreSQL
8.4, commit a77eaa6a to be more precise. Currently GPDB's ExecInsert
uses ExecSlotFetch*() functions to get the tuple from the slot, while in
the upstream, it makes a modifiable copy with ExecMaterializeSlot(). That's
OK as the code stands, because there's always a "junk filter" that ensures
that the slot doesn't point directly to an on-disk tuple. But commit
a77eaa6a will change that, so we have to start being more careful.

This does fix an existing bug, namely that if you UPDATE an AO table with
OIDs, the OIDs currently change (github issue #3732). Add a test case for
that.

More detailed breakdown of the changes:

* In ExecInsert, create a writeable copy of the tuple when we're about
  to modify it, so that we don't accidentally modify an existing on-disk
  tuple. By calling ExecMaterializeSlot().

* In ExecInsert, track the OID of the tuple we're about to insert in a
  local variable, when we call the BEFORE ROW triggers, because we don't
  have a "tuple" yet.

* Add ExecMaterializeSlot() function, like in the upstream, because we now
  need it in ExecInsert. Refactor ExecFetchSlotHeapTuple to use
  ExecMaterializeSlot(), like in upstream.

* Cherry-pick bug fix commit 3d02cae3 from upstream. We would get that
  soon anyway as part of the merge, but we'll soon have test failures if
  we don't fix it immediately.

* Change the API of appendonly_insert(), so that it takes the new OID as
  argument, instead of extracting it from the passed-in MemTuple. With this
  change, appendonly_insert() is guaranteed to not modify the passed-in
  MemTuple, so we don't need the equivalent of ExecMaterializeSlot() for
  MemTuples.

* Also change the API of appendonly_insert() so that it returns the new OID
  of the inserted tuple, like heap_insert() does. Most callers ignore the
  return value, so this way they don't need to pass a dummy pointer
  argument.

* Add test case for the case that a BEFORE ROW trigger sets the OID of
  a tuple we're about to insert.

This is based on earlier patches against the 8.4 merge iteration3 branch by
Jacob and Max.

src/backend/access/appendonly/appendonly_compaction.c

@@ -281,7 +281,7 @@ AppendOnlyMoveTuple(MemTuple tuple,
 	tupleOid = MemTupleGetOid(tuple, mt_bind);
 	appendonly_insert(insertDesc,
 					  tuple,
-					  &tupleOid,
+					  tupleOid,
 					  &newAoTupleId);
 
 	/* insert index' tuples if needed */

src/backend/access/appendonly/appendonlyam.c

@@ -2652,11 +2652,9 @@ appendonly_update(AppendOnlyUpdateDesc aoUpdateDesc,
 	if (result != HeapTupleMayBeUpdated)
 		return result;
 
-	Oid			newOid = InvalidOid;	/* new oid should be old oid */
-
 	appendonly_insert(aoUpdateDesc->aoInsertDesc,
 					  memTuple,
-					  &newOid,
+					  InvalidOid,	/* new oid should be old oid */
 					  newAoTupleId);
 
 	return result;
@@ -2892,11 +2890,13 @@ aoInsertDesc->appendOnlyMetaDataSnapshot, //CONCERN:Safe to assume all block dir
   * The output parameter tupleOid 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;
+  *
+  * Unlike heap_insert(), this function doesn't scribble on the input tuple.
   */
-void
+Oid
 appendonly_insert(AppendOnlyInsertDesc aoInsertDesc,
 				  MemTuple instup,
-				  Oid *tupleOid,
+				  Oid tupleOid,
 				  AOTupleId *aoTupleId)
 {
 	Relation	relation = aoInsertDesc->aoi_rel;
@@ -2919,19 +2919,6 @@ appendonly_insert(AppendOnlyInsertDesc aoInsertDesc,
 #endif
 
 	Insist(RelationIsAoRows(relation));
-	if (relation->rd_rel->relhasoids)
-	{
-		/*
-		 * If the object id of this tuple has already been assigned, trust the
-		 * caller.	There are a couple of ways this can happen.  At initial db
-		 * creation, the backend program sets oids for tuples. When we define
-		 * an index, we set the oid.  Finally, in the future, we may allow
-		 * users to set their own object ids in order to support a persistent
-		 * object store (objects need to contain pointers to one another).
-		 */
-		if (!OidIsValid(MemTupleGetOid(instup, aoInsertDesc->mt_bind)))
-			MemTupleSetOid(instup, aoInsertDesc->mt_bind, GetNewOid(relation));
-	}
 
 	if (aoInsertDesc->useNoToast)
 		need_toast = false;
@@ -2955,6 +2942,32 @@ appendonly_insert(AppendOnlyInsertDesc aoInsertDesc,
 	else
 		tup = instup;
 
+	if (relation->rd_rel->relhasoids)
+	{
+		/*
+		 * Don't modify the input tuple, so make a copy unless we already
+		 * made one. I'm not sure if the input tuple can point to any
+		 * permanent storage, so modifying it might be harmless, but better
+		 * safe than sorry. An AO table with OIDs is a weird beast anyway,
+		 * so performance of this case isn't important.
+		 */
+		if (tup == instup)
+			tup = memtuple_copy_to(instup, NULL, NULL);
+
+		/*
+		 * If the object id of this tuple has already been assigned, trust the
+		 * caller.	There are a couple of ways this can happen.  At initial db
+		 * creation, the backend program sets oids for tuples. When we define
+		 * an index, we set the oid.  Finally, in the future, we may allow
+		 * users to set their own object ids in order to support a persistent
+		 * object store (objects need to contain pointers to one another).
+		 */
+		if (!OidIsValid(tupleOid))
+			tupleOid = GetNewOid(relation);
+
+		MemTupleSetOid(tup, aoInsertDesc->mt_bind, tupleOid);
+	}
+
 	/*
 	 * get space to insert our next item (tuple)
 	 */
@@ -3101,7 +3114,7 @@ appendonly_insert(AppendOnlyInsertDesc aoInsertDesc,
 
 	Assert(aoInsertDesc->numSequences >= 0);
 
-	*tupleOid = MemTupleGetOid(tup, aoInsertDesc->mt_bind);
+	tupleOid = MemTupleGetOid(tup, aoInsertDesc->mt_bind);
 
 	AOTupleIdInit_Init(aoTupleId);
 	AOTupleIdInit_segmentFileNum(aoTupleId, aoInsertDesc->cur_segno);
@@ -3136,6 +3149,8 @@ appendonly_insert(AppendOnlyInsertDesc aoInsertDesc,
 
 	if (tup != instup)
 		pfree(tup);
+
+	return tupleOid;
 }
 
 /*

src/backend/access/external/fileam.c

@@ -584,6 +584,7 @@ external_insert_init(Relation rel)
  * - transaction information is of no interest.
  * - tuples are sent always to the destination (local file or remote target).
  *
+ * Like heap_insert(), this function can modify the input tuple!
  */
 Oid
 external_insert(ExternalInsertDesc extInsertDesc, HeapTuple instup)

src/backend/commands/copy.c

@@ -4907,13 +4907,12 @@ PROCESS_SEGMENT_DATA:
 					 */
 					if (relstorage == RELSTORAGE_AOROWS)
 					{
-						Oid			tupleOid;
 						MemTuple	mtuple;
 
 						mtuple = ExecFetchSlotMemTuple(slot, false);
 
 						/* inserting into an append only relation */
-						appendonly_insert(resultRelInfo->ri_aoInsertDesc, mtuple, &tupleOid, (AOTupleId *) &insertedTid);
+						appendonly_insert(resultRelInfo->ri_aoInsertDesc, mtuple, InvalidOid, (AOTupleId *) &insertedTid);
 					}
 					else if (relstorage == RELSTORAGE_AOCOLS)
 					{

src/backend/commands/tablecmds.c

@@ -5337,10 +5337,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap)
 				/* Write the tuple out to the new relation */
 				if (newrel)
 				{
-					Oid 		tupleOid;
 					AOTupleId	aoTupleId;
 					
-					appendonly_insert(aoInsertDesc, mtuple, &tupleOid, &aoTupleId);
+					appendonly_insert(aoInsertDesc, mtuple, InvalidOid, &aoTupleId);
 				}
 
 				ResetExprContext(econtext);
@@ -14322,7 +14321,6 @@ split_rows(Relation intoa, Relation intob, Relation temprel)
 		else if (RelationIsAoRows(targetRelation))
 		{
 			MemTuple	mtuple;
-			Oid			tupleOid;
 
 			if (!(*targetAODescPtr))
 			{
@@ -14333,7 +14331,7 @@ split_rows(Relation intoa, Relation intob, Relation temprel)
 			}
 
 			mtuple = ExecFetchSlotMemTuple(targetSlot, false);
-			appendonly_insert(*targetAODescPtr, mtuple, &tupleOid, &aoTupleId);
+			appendonly_insert(*targetAODescPtr, mtuple, InvalidOid, &aoTupleId);
 
 			/* cache TID for later updating of indexes */
 			tid = (ItemPointer) &aoTupleId;

src/backend/executor/execMain.c

@@ -3080,6 +3080,7 @@ ExecInsert(TupleTableSlot *slot,
 	bool		rel_is_aocols = false;
 	bool		rel_is_external = false;
 	ItemPointerData lastTid;
+	Oid			tuple_oid = InvalidOid;
 
 	/*
 	 * get information on the (current) result relation
@@ -3144,6 +3145,81 @@ ExecInsert(TupleTableSlot *slot,
 	rel_is_aorows = RelationIsAoRows(resultRelationDesc);
 	rel_is_external = RelationIsExternal(resultRelationDesc);
 
+	/*
+	 * Prepare the right kind of "insert desc".
+	 */
+	if (rel_is_aorows)
+	{
+		if (resultRelInfo->ri_aoInsertDesc == NULL)
+		{
+			/* Set the pre-assigned fileseg number to insert into */
+			ResultRelInfoSetSegno(resultRelInfo, estate->es_result_aosegnos);
+
+			resultRelInfo->ri_aoInsertDesc =
+				appendonly_insert_init(resultRelationDesc,
+									   resultRelInfo->ri_aosegno,
+									   false);
+		}
+	}
+	else if (rel_is_aocols)
+	{
+		if (resultRelInfo->ri_aocsInsertDesc == NULL)
+		{
+			ResultRelInfoSetSegno(resultRelInfo, estate->es_result_aosegnos);
+			resultRelInfo->ri_aocsInsertDesc = aocs_insert_init(resultRelationDesc,
+																resultRelInfo->ri_aosegno, false);
+		}
+	}
+	else if (rel_is_external)
+	{
+		if (resultRelInfo->ri_extInsertDesc == NULL)
+			resultRelInfo->ri_extInsertDesc = external_insert_init(resultRelationDesc);
+	}
+
+	/*
+	 * If the result relation has OIDs, force the tuple's OID to zero so that
+	 * heap_insert will assign a fresh OID.  Usually the OID already will be
+	 * zero at this point, but there are corner cases where the plan tree can
+	 * return a tuple extracted literally from some table with the same
+	 * rowtype.
+	 *
+	 * XXX if we ever wanted to allow users to assign their own OIDs to new
+	 * rows, this'd be the place to do it.  For the moment, we make a point
+	 * of doing this before calling triggers, so that a user-supplied trigger
+	 * could hack the OID if desired.
+	 *
+	 * GPDB: In PostgreSQL, here we set the Oid in the HeapTuple, which is a
+	 * local copy at this point. But in GPDB, we don't materialize the tuple
+	 * yet, because we might need a MemTuple or a HeapTuple depending on
+	 * what kind of a table this is (or neither for an AOCS table, since
+	 * aocs_insert() works directly off the slot). So we keep the Oid in a
+	 * local variable for now, and only set it in the tuple just before the
+	 * call to heap/appendonly/external_insert().
+	 */
+	if (resultRelationDesc->rd_rel->relhasoids)
+	{
+		tuple_oid = InvalidOid;
+
+		/*
+		 * But if this is really an UPDATE, try to preserve the old OID.
+		 */
+		if (isUpdate)
+		{
+			GenericTuple gtuple;
+
+			gtuple = ExecFetchSlotGenericTuple(slot, false);
+
+			if (!is_memtuple(gtuple))
+				tuple_oid = HeapTupleGetOid((HeapTuple) gtuple);
+			else
+			{
+				if (resultRelInfo->ri_aoInsertDesc)
+					tuple_oid = MemTupleGetOid((MemTuple) gtuple,
+											   resultRelInfo->ri_aoInsertDesc->mt_bind);
+			}
+		}
+	}
+
 	slot = reconstructMatchingTupleSlot(slot, resultRelInfo);
 
 	if (rel_is_external &&
@@ -3165,7 +3241,9 @@ ExecInsert(TupleTableSlot *slot,
 		HeapTuple	newtuple;
 		HeapTuple	tuple;
 
-		tuple = ExecFetchSlotHeapTuple(slot);
+		tuple = ExecMaterializeSlot(slot);
+		if (resultRelationDesc->rd_rel->relhasoids)
+			HeapTupleSetOid(tuple, tuple_oid);
 
 		newtuple = ExecBRInsertTriggers(estate, resultRelInfo, tuple);
 
@@ -3188,6 +3266,13 @@ ExecInsert(TupleTableSlot *slot,
 			newslot->tts_tableOid = slot->tts_tableOid; /* for constraints */
 			slot = newslot;
 			tuple = newtuple;
+
+			/*
+			 * since we keep the OID in a separate variable, also update that,
+			 * in case the trigger set it.
+			 */
+			if (resultRelationDesc->rd_rel->relhasoids)
+				tuple_oid = HeapTupleGetOid(newtuple);
 		}
 	}
 
@@ -3218,11 +3303,10 @@ ExecInsert(TupleTableSlot *slot,
 				appendonly_insert_init(resultRelationDesc,
 									   resultRelInfo->ri_aosegno,
 									   false);
-
 		}
 
 		mtuple = ExecFetchSlotMemTuple(slot, false);
-		appendonly_insert(resultRelInfo->ri_aoInsertDesc, mtuple, &newId, (AOTupleId *) &lastTid);
+		newId = appendonly_insert(resultRelInfo->ri_aoInsertDesc, mtuple, tuple_oid, (AOTupleId *) &lastTid);
 	}
 	else if (rel_is_aocols)
 	{
@@ -3244,7 +3328,13 @@ ExecInsert(TupleTableSlot *slot,
 		if (resultRelInfo->ri_extInsertDesc == NULL)
 			resultRelInfo->ri_extInsertDesc = external_insert_init(resultRelationDesc);
 
-		tuple = ExecFetchSlotHeapTuple(slot);
+		/*
+		 * get the heap tuple out of the tuple table slot, making sure we have a
+		 * writable copy. (external_insert() can scribble on the tuple)
+		 */
+		tuple = ExecMaterializeSlot(slot);
+		if (resultRelationDesc->rd_rel->relhasoids)
+			HeapTupleSetOid(tuple, tuple_oid);
 
 		newId = external_insert(resultRelInfo->ri_extInsertDesc, tuple);
 		ItemPointerSetInvalid(&lastTid);
@@ -3255,7 +3345,13 @@ ExecInsert(TupleTableSlot *slot,
 
 		Insist(rel_is_heap);
 
-		tuple = ExecFetchSlotHeapTuple(slot);
+		/*
+		 * get the heap tuple out of the tuple table slot, making sure we have a
+		 * writable copy. (heap_insert() will scribble on the tuple)
+		 */
+		tuple = ExecMaterializeSlot(slot);
+		if (resultRelationDesc->rd_rel->relhasoids)
+			HeapTupleSetOid(tuple, tuple_oid);
 
 		newId = heap_insert(resultRelationDesc,
 							tuple,
@@ -3282,7 +3378,7 @@ ExecInsert(TupleTableSlot *slot,
 		resultRelInfo->ri_TrigDesc->n_after_row[TRIGGER_EVENT_INSERT] > 0 &&
 		!isUpdate)
 	{
-		HeapTuple tuple = ExecFetchSlotHeapTuple(slot);
+		HeapTuple tuple = ExecMaterializeSlot(slot);
 
 		Assert(planGen == PLANGEN_PLANNER);
 
@@ -3821,7 +3917,7 @@ lreplace:;
 		{
 			HeapTuple tuple;
 
-			tuple = ExecFetchSlotHeapTuple(slot);
+			tuple = ExecMaterializeSlot(slot);
 
 			result = heap_update(resultRelationDesc, tupleid, tuple,
 							 &update_ctid, &update_xmax,
@@ -3925,13 +4021,11 @@ lreplace:;
 		/*
 		 * Persistent metadata path.
 		 */
-		persistentTuple = ExecCopySlotHeapTuple(slot);
+		persistentTuple = ExecMaterializeSlot(slot);
 		persistentTuple->t_self = *tupleid;
 
 		frozen_heap_inplace_update(resultRelationDesc, persistentTuple);
 		wasHotUpdate = false;
-
-		heap_freetuple(persistentTuple);
 	}
 
 	(estate->es_processed)++;
@@ -3961,7 +4055,7 @@ lreplace:;
 		resultRelInfo->ri_TrigDesc->n_after_row[TRIGGER_EVENT_UPDATE] > 0 &&
 		rel_is_heap)
 	{
-		HeapTuple tuple = ExecFetchSlotHeapTuple(slot);
+		HeapTuple tuple = ExecMaterializeSlot(slot);
 
 		ExecARUpdateTriggers(estate, resultRelInfo, tupleid, tuple);
 	}
@@ -5043,13 +5137,12 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
 	if (RelationIsAoRows(into_rel))
 	{
 		MemTuple	tuple = ExecCopySlotMemTuple(slot);
-		Oid			tupleOid;
 		AOTupleId	aoTupleId;
 
 		if (myState->ao_insertDesc == NULL)
 			myState->ao_insertDesc = appendonly_insert_init(into_rel, RESERVED_SEGNO, false);
 
-		appendonly_insert(myState->ao_insertDesc, tuple, &tupleOid, &aoTupleId);
+		appendonly_insert(myState->ao_insertDesc, tuple, InvalidOid, &aoTupleId);
 		pfree(tuple);
 	}
 	else if (RelationIsAoCols(into_rel))
@@ -5061,7 +5154,19 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
 	}
 	else
 	{
-		HeapTuple	tuple = ExecCopySlotHeapTuple(slot);
+		HeapTuple	tuple;
+
+		/*
+		 * get the heap tuple out of the tuple table slot, making sure we have a
+		 * writable copy
+		 */
+		tuple = ExecMaterializeSlot(slot);
+
+		/*
+		 * force assignment of new OID (see comments in ExecInsert)
+		 */
+		if (into_rel->rd_rel->relhasoids)
+			HeapTupleSetOid(tuple, InvalidOid);
 
 		heap_insert(into_rel,
 					tuple,
@@ -5071,8 +5176,6 @@ intorel_receive(TupleTableSlot *slot, DestReceiver *self)
 					GetCurrentTransactionId());
 
 		estate->es_into_relation_last_heap_tid = tuple->t_self;
-
-		heap_freetuple(tuple);
 	}
 
 	/* We know this is a newly created relation, so there are no indexes */

src/backend/executor/execTuples.c

@@ -703,9 +703,6 @@ ExecCopySlotMemTupleTo(TupleTableSlot *slot, MemoryContext pctxt, char *dest, un
 HeapTuple
 ExecFetchSlotHeapTuple(TupleTableSlot *slot)
 {
-	uint32 tuplen;
-	HeapTuple htup;
-
 	/*
 	 * sanity checks
 	 */
@@ -717,29 +714,10 @@ ExecFetchSlotHeapTuple(TupleTableSlot *slot)
 	if(slot->PRIVATE_tts_heaptuple)
 		return slot->PRIVATE_tts_heaptuple;
 
-	slot_getallattrs(slot);
-
-	Assert(TupHasVirtualTuple(slot));
-	Assert(slot->PRIVATE_tts_nvalid == slot->tts_tupleDescriptor->natts);
-
-	tuplen = slot->PRIVATE_tts_htup_buf_len;
-	htup = heaptuple_form_to(slot->tts_tupleDescriptor, slot_get_values(slot), slot_get_isnull(slot),
-			slot->PRIVATE_tts_htup_buf, &tuplen);
-	
-	if(!htup)
-	{
-		if(slot->PRIVATE_tts_htup_buf)
-			pfree(slot->PRIVATE_tts_htup_buf);
-		slot->PRIVATE_tts_htup_buf = (HeapTuple) MemoryContextAlloc(slot->tts_mcxt, tuplen);
-		slot->PRIVATE_tts_htup_buf_len = tuplen;
-
-		htup = heaptuple_form_to(slot->tts_tupleDescriptor, slot_get_values(slot), slot_get_isnull(slot),
-			slot->PRIVATE_tts_htup_buf, &tuplen);
-		Assert(htup);
-	}
-
-	slot->PRIVATE_tts_heaptuple = htup;
-	return htup;
+	/*
+	 * Otherwise materialize the slot...
+	 */
+	return ExecMaterializeSlot(slot);
 }
 
 /* --------------------------------
@@ -802,6 +780,69 @@ ExecFetchSlotMemTuple(TupleTableSlot *slot, bool inline_toast)
 	return newTuple;
 }
 
+/* --------------------------------
+ *		ExecMaterializeSlot
+ *			Force a slot into the "materialized" state.
+ *
+ *		This causes the slot's tuple to be a local copy not dependent on
+ *		any external storage.  A pointer to the contained tuple is returned.
+ *
+ *		A typical use for this operation is to prepare a computed tuple
+ *		for being stored on disk.  The original data may or may not be
+ *		virtual, but in any case we need a private copy for heap_insert
+ *		to scribble on.
+ * --------------------------------
+ */
+HeapTuple
+ExecMaterializeSlot(TupleTableSlot *slot)
+{
+	uint32		tuplen;
+	HeapTuple	htup;
+
+	/*
+	 * sanity checks
+	 */
+	Assert(!TupIsNull(slot));
+
+	/*
+	 * If we have a regular physical tuple, and it's locally palloc'd, we have
+	 * nothing to do.
+	 */
+	if (slot->PRIVATE_tts_heaptuple &&
+		(TupShouldFree(slot) || slot->PRIVATE_tts_heaptuple == slot->PRIVATE_tts_htup_buf))
+		return slot->PRIVATE_tts_heaptuple;
+
+	/*
+	 * Otherwise, copy or build a physical tuple, and store it into the slot.
+	 */
+	slot_getallattrs(slot);
+
+	Assert(slot->PRIVATE_tts_nvalid == slot->tts_tupleDescriptor->natts);
+
+	tuplen = slot->PRIVATE_tts_htup_buf_len;
+	htup = heaptuple_form_to(slot->tts_tupleDescriptor, slot_get_values(slot), slot_get_isnull(slot),
+			slot->PRIVATE_tts_htup_buf, &tuplen);
+
+	if (!htup)
+	{
+		/* enlarge the buffer and retry */
+		if (slot->PRIVATE_tts_htup_buf)
+			pfree(slot->PRIVATE_tts_htup_buf);
+		slot->PRIVATE_tts_htup_buf = (HeapTuple) MemoryContextAlloc(slot->tts_mcxt, tuplen);
+		slot->PRIVATE_tts_htup_buf_len = tuplen;
+
+		htup = heaptuple_form_to(slot->tts_tupleDescriptor,
+								 slot_get_values(slot),
+								 slot_get_isnull(slot),
+								 slot->PRIVATE_tts_htup_buf,
+								 &tuplen);
+		Assert(htup);
+	}
+
+	slot->PRIVATE_tts_heaptuple = htup;
+	return htup;
+}
+
 /* --------------------------------
  *		ExecCopySlot
  *			Copy the source slot's contents into the destination slot.

src/include/cdb/cdbappendonlyam.h

@@ -351,10 +351,10 @@ extern bool appendonly_fetch(
 	TupleTableSlot *slot);
 extern void appendonly_fetch_finish(AppendOnlyFetchDesc aoFetchDesc);
 extern AppendOnlyInsertDesc appendonly_insert_init(Relation rel, int segno, bool update_mode);
-extern void appendonly_insert(
+extern Oid appendonly_insert(
 		AppendOnlyInsertDesc aoInsertDesc, 
 		MemTuple instup, 
-		Oid *tupleOid, 
+		Oid tupleOid,
 		AOTupleId *aoTupleId);
 extern void appendonly_insert_finish(AppendOnlyInsertDesc aoInsertDesc);
 extern BlockNumber RelationGuessNumberOfBlocks(double totalbytes);

src/include/executor/tuptable.h

@@ -429,6 +429,7 @@ ExecCopyGenericTuple(TupleTableSlot *slot)
 	return (GenericTuple) ExecCopySlotMemTuple(slot);
 }
 
+extern HeapTuple ExecMaterializeSlot(TupleTableSlot *slot);
 extern TupleTableSlot *ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot);
 
 extern void ExecModifyMemTuple(TupleTableSlot *slot, Datum *values, bool *isnull, bool *doRepl);

src/test/regress/expected/bfv_dml.out

@@ -250,3 +250,39 @@ EXECUTE PROCEDURE bfv_dml_error_func();
 UPDATE bfv_dml_trigger_test SET t = 'bar';
 UPDATE bfv_dml_trigger_test SET id = id + 1;
 ERROR:  Cannot parallelize an UPDATE statement that updates the distribution columns
+--
+-- Verify that ExecInsert doesn't scribble on the old tuple, when the new
+-- tuple comes directly from the old table.
+--
+CREATE TABLE execinsert_test (id int4, t text) DISTRIBUTED BY (id);
+INSERT INTO execinsert_test values (1, 'foo');
+-- Insert another identical tuple, but roll it back. If the insertion
+-- incorrectly modified the xmin on the old tuple, then it will become
+-- invisible when we roll back.
+begin;
+INSERT INTO execinsert_test select * FROM execinsert_test;
+rollback;
+select * from execinsert_test;
+ id |  t  
+----+-----
+  1 | foo
+(1 row)
+
+drop table execinsert_test;
+-- Repeat with a hacked master-only table, just in case the planner decides
+-- to add a Motion node or something that hides the problem otherwise.
+CREATE TABLE execinsert_test (id int4, t text) DISTRIBUTED BY (id);
+set allow_system_table_mods='DML';
+delete from gp_distribution_policy where localoid='execinsert_test'::regclass;
+reset allow_system_table_mods;
+INSERT INTO execinsert_test values (1, 'foo');
+begin;
+INSERT INTO execinsert_test select * FROM execinsert_test;
+rollback;
+select * from execinsert_test;
+ id |  t  
+----+-----
+  1 | foo
+(1 row)
+
+drop table execinsert_test;

src/test/regress/expected/bfv_dml_optimizer.out

@@ -267,3 +267,39 @@ FOR EACH ROW
 EXECUTE PROCEDURE bfv_dml_error_func();
 UPDATE bfv_dml_trigger_test SET t = 'bar';
 UPDATE bfv_dml_trigger_test SET id = id + 1;
+--
+-- Verify that ExecInsert doesn't scribble on the old tuple, when the new
+-- tuple comes directly from the old table.
+--
+CREATE TABLE execinsert_test (id int4, t text) DISTRIBUTED BY (id);
+INSERT INTO execinsert_test values (1, 'foo');
+-- Insert another identical tuple, but roll it back. If the insertion
+-- incorrectly modified the xmin on the old tuple, then it will become
+-- invisible when we roll back.
+begin;
+INSERT INTO execinsert_test select * FROM execinsert_test;
+rollback;
+select * from execinsert_test;
+ id |  t  
+----+-----
+  1 | foo
+(1 row)
+
+drop table execinsert_test;
+-- Repeat with a hacked master-only table, just in case the planner decides
+-- to add a Motion node or something that hides the problem otherwise.
+CREATE TABLE execinsert_test (id int4, t text) DISTRIBUTED BY (id);
+set allow_system_table_mods='DML';
+delete from gp_distribution_policy where localoid='execinsert_test'::regclass;
+reset allow_system_table_mods;
+INSERT INTO execinsert_test values (1, 'foo');
+begin;
+INSERT INTO execinsert_test select * FROM execinsert_test;
+rollback;
+select * from execinsert_test;
+ id |  t  
+----+-----
+  1 | foo
+(1 row)
+
+drop table execinsert_test;

src/test/regress/expected/qp_dml_oids.out

@@ -298,3 +298,16 @@ SELECT COUNT(distinct oid) FROM dml_ao where a = 10;
      2
 (1 row)
 
+--
+-- Check that new OIDs are generated even if the tuple being inserted came from
+-- the same relation and segment.
+--
+INSERT INTO dml_ao VALUES (11, 1, 'foo');
+INSERT INTO dml_ao VALUES (11, 2, 'bar');
+INSERT INTO dml_ao SELECT * FROM dml_ao WHERE a = 11 LIMIT 1;
+SELECT COUNT(DISTINCT oid) FROM dml_ao WHERE a = 11; -- all three rows should have different OID
+ count 
+-------
+     3
+(1 row)
+

src/test/regress/expected/qp_dml_oids_optimizer.out

@@ -296,3 +296,16 @@ SELECT COUNT(distinct oid) FROM dml_ao where a = 10;
      2
 (1 row)
 
+--
+-- Check that new OIDs are generated even if the tuple being inserted came from
+-- the same relation and segment.
+--
+INSERT INTO dml_ao VALUES (11, 1, 'foo');
+INSERT INTO dml_ao VALUES (11, 2, 'bar');
+INSERT INTO dml_ao SELECT * FROM dml_ao WHERE a = 11 LIMIT 1;
+SELECT COUNT(DISTINCT oid) FROM dml_ao WHERE a = 11; -- all three rows should have different OID
+ count 
+-------
+     3
+(1 row)
+

src/test/regress/greenplum_schedule

@@ -113,7 +113,7 @@ test: tuple_serialization
 # temp tables
 test: bfv_cte bfv_joins bfv_subquery bfv_planner bfv_legacy bfv_temp bfv_dml
 
-test: qp_olap_mdqa qp_misc gp_recursive_cte qp_dml_joins qp_dml_oids
+test: qp_olap_mdqa qp_misc gp_recursive_cte qp_dml_joins qp_dml_oids trigger_sets_oid
 
 test: qp_misc_jiras qp_with_clause qp_executor qp_olap_windowerr qp_olap_window qp_derived_table qp_bitmapscan session_level_memory_consumption qp_dropped_cols
 test: qp_with_functional_inlining qp_with_functional_noinlining

src/test/regress/input/trigger_sets_oid.source

+--
+-- Test setting the OID of a tuple in a BEFORE ROW INSERT trigger.
+--
+-- There's a comment in ExecInsert that says that that should work, although there
+-- is no code in PostgreSQL to test that. So let's test it.
+--
+
+CREATE OR REPLACE FUNCTION trigger_return_new_oid()
+RETURNS trigger
+LANGUAGE C VOLATILE EXECUTE ON MASTER AS '@abs_builddir@/regress@DLSUFFIX@', 'trigger_udf_return_new_oid';
+
+
+-- Heap table
+CREATE TABLE trigger_oid_tab(id int8, v text) WITH (oids = true);
+
+CREATE TRIGGER t BEFORE INSERT ON trigger_oid_tab
+FOR EACH ROW EXECUTE PROCEDURE trigger_return_new_oid(12345);
+
+INSERT INTO trigger_oid_tab values (1, 'foo');
+SELECT oid, * FROM trigger_oid_tab;
+
+DROP TABLE trigger_oid_tab;
+
+-- AO table
+CREATE TABLE trigger_oid_tab(id int8, v text) WITH (oids = true, appendonly = true);
+
+CREATE TRIGGER t BEFORE INSERT ON trigger_oid_tab
+FOR EACH ROW EXECUTE PROCEDURE trigger_return_new_oid(12345);
+
+INSERT INTO trigger_oid_tab values (1, 'foo');
+SELECT oid, * FROM trigger_oid_tab;
+
+DROP TABLE trigger_oid_tab;
+
+-- Column-oriented AO tables don't support OIDs, so no need to test that.

src/test/regress/output/trigger_sets_oid.source

+--
+-- Test setting the OID of a tuple in a BEFORE ROW INSERT trigger.
+--
+-- There's a comment in ExecInsert that says that that should work, although there
+-- is no code in PostgreSQL to test that. So let's test it.
+--
+CREATE OR REPLACE FUNCTION trigger_return_new_oid()
+RETURNS trigger
+LANGUAGE C VOLATILE EXECUTE ON MASTER AS '@abs_builddir@/regress@DLSUFFIX@', 'trigger_udf_return_new_oid';
+-- Heap table
+CREATE TABLE trigger_oid_tab(id int8, v text) WITH (oids = true);
+NOTICE:  Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'id' as the Greenplum Database data distribution key for this table.
+HINT:  The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew.
+NOTICE:  OIDS=TRUE is not recommended for user-created tables. Use OIDS=FALSE to prevent wrap-around of the OID counter
+CREATE TRIGGER t BEFORE INSERT ON trigger_oid_tab
+FOR EACH ROW EXECUTE PROCEDURE trigger_return_new_oid(12345);
+INSERT INTO trigger_oid_tab values (1, 'foo');
+NOTICE:  trigger_udf_return_new_oid assigned OID 12345 to the new tuple  (seg0 127.0.0.1:40000 pid=25965)
+SELECT oid, * FROM trigger_oid_tab;
+  oid  | id |  v  
+-------+----+-----
+ 12345 |  1 | foo
+(1 row)
+
+DROP TABLE trigger_oid_tab;
+-- AO table
+CREATE TABLE trigger_oid_tab(id int8, v text) WITH (oids = true, appendonly = true);
+NOTICE:  Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'id' as the Greenplum Database data distribution key for this table.
+HINT:  The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew.
+NOTICE:  OIDS=TRUE is not recommended for user-created tables. Use OIDS=FALSE to prevent wrap-around of the OID counter
+CREATE TRIGGER t BEFORE INSERT ON trigger_oid_tab
+FOR EACH ROW EXECUTE PROCEDURE trigger_return_new_oid(12345);
+INSERT INTO trigger_oid_tab values (1, 'foo');
+NOTICE:  trigger_udf_return_new_oid assigned OID 12345 to the new tuple  (seg0 127.0.0.1:40000 pid=25965)
+SELECT oid, * FROM trigger_oid_tab;
+  oid  | id |  v  
+-------+----+-----
+ 12345 |  1 | foo
+(1 row)
+
+DROP TABLE trigger_oid_tab;
+-- Column-oriented AO tables don't support OIDs, so no need to test that.

src/test/regress/regress_gp.c

@@ -108,6 +108,7 @@ extern Datum check_foreign_key(PG_FUNCTION_ARGS);
 extern Datum autoinc(PG_FUNCTION_ARGS);
 static EPlan *find_plan(char *ident, EPlan ** eplan, int *nplans);
 
+extern Datum trigger_udf_return_new_oid(PG_FUNCTION_ARGS);
 
 
 PG_FUNCTION_INFO_V1(multiset_scalar_null);
@@ -1903,3 +1904,51 @@ find_plan(char *ident, EPlan ** eplan, int *nplans)
 
 	return (newp);
 }
+
+
+/*
+ * trigger_udf_return_new_oid
+ *
+ * A helper function to assign a specific OID to a tuple on INSERT.
+ */
+PG_FUNCTION_INFO_V1(trigger_udf_return_new_oid);
+Datum
+trigger_udf_return_new_oid(PG_FUNCTION_ARGS)
+{
+	TriggerData *trigdata = (TriggerData *) fcinfo->context;
+	Trigger    *trigger;
+	char	  **args;
+	HeapTuple	input_tuple;
+	HeapTuple	ret_tuple;
+	Oid			new_oid;
+
+	if (!CALLED_AS_TRIGGER(fcinfo))
+		elog(ERROR, "not fired by trigger manager");
+	if (!TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
+		elog(ERROR, "cannot process STATEMENT events");
+	if (!TRIGGER_FIRED_BEFORE(trigdata->tg_event))
+		elog(ERROR, "must be fired before event");
+
+	if (!TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
+		elog(ERROR, "trigger_udf_return_new_oid() called for a non-INSERT");
+
+	/*
+	 * Get the argument. (Trigger functions receive their arguments
+	 * differently than normal functions.)
+	 */
+	trigger = trigdata->tg_trigger;
+	if (trigger->tgnargs != 1)
+		elog(ERROR, "trigger_udf_return_new_oid called with invalid number of arguments (%d, expected 1)",
+			 trigger->tgnargs);
+	args = trigger->tgargs;
+	new_oid = DatumGetObjectId(DirectFunctionCall1(oidin,
+												   CStringGetDatum(args[0])));
+
+	elog(NOTICE, "trigger_udf_return_new_oid assigned OID %u to the new tuple", new_oid);
+
+	input_tuple = trigdata->tg_trigtuple;
+	ret_tuple = heap_copytuple(input_tuple);
+	HeapTupleSetOid(ret_tuple, new_oid);
+
+	return PointerGetDatum(ret_tuple);
+}

src/test/regress/sql/bfv_dml.sql

@@ -164,3 +164,38 @@ EXECUTE PROCEDURE bfv_dml_error_func();
 
 UPDATE bfv_dml_trigger_test SET t = 'bar';
 UPDATE bfv_dml_trigger_test SET id = id + 1;
+
+
+--
+-- Verify that ExecInsert doesn't scribble on the old tuple, when the new
+-- tuple comes directly from the old table.
+--
+CREATE TABLE execinsert_test (id int4, t text) DISTRIBUTED BY (id);
+
+INSERT INTO execinsert_test values (1, 'foo');
+
+-- Insert another identical tuple, but roll it back. If the insertion
+-- incorrectly modified the xmin on the old tuple, then it will become
+-- invisible when we roll back.
+begin;
+INSERT INTO execinsert_test select * FROM execinsert_test;
+rollback;
+select * from execinsert_test;
+
+drop table execinsert_test;
+
+-- Repeat with a hacked master-only table, just in case the planner decides
+-- to add a Motion node or something that hides the problem otherwise.
+
+CREATE TABLE execinsert_test (id int4, t text) DISTRIBUTED BY (id);
+set allow_system_table_mods='DML';
+delete from gp_distribution_policy where localoid='execinsert_test'::regclass;
+reset allow_system_table_mods;
+
+INSERT INTO execinsert_test values (1, 'foo');
+begin;
+INSERT INTO execinsert_test select * FROM execinsert_test;
+rollback;
+select * from execinsert_test;
+
+drop table execinsert_test;

src/test/regress/sql/qp_dml_oids.sql

@@ -162,3 +162,12 @@ INSERT INTO dml_ao (a, b, c) VALUES (10, 1, repeat('x', 50000));
 INSERT INTO dml_ao (a, b, c) VALUES (10, 2, repeat('x', 50000));
 
 SELECT COUNT(distinct oid) FROM dml_ao where a = 10;
+
+--
+-- Check that new OIDs are generated even if the tuple being inserted came from
+-- the same relation and segment.
+--
+INSERT INTO dml_ao VALUES (11, 1, 'foo');
+INSERT INTO dml_ao VALUES (11, 2, 'bar');
+INSERT INTO dml_ao SELECT * FROM dml_ao WHERE a = 11 LIMIT 1;
+SELECT COUNT(DISTINCT oid) FROM dml_ao WHERE a = 11; -- all three rows should have different OID