Throw error if expiring tuple is again updated or deleted.

This prevents surprising behavior when a FOR EACH ROW trigger
BEFORE UPDATE or BEFORE DELETE directly or indirectly updates or
deletes the the old row.  Prior to this patch the requested action
on the row could be silently ignored while all triggered actions
based on the occurence of the requested action could be committed.
One example of how this could happen is if the BEFORE DELETE
trigger for a "parent" row deleted "children" which had trigger
functions to update summary or status data on the parent.

This also prevents similar surprising problems if the query has a
volatile function which updates a target row while it is already
being updated.

There are related issues present in FOR UPDATE cursors and READ
COMMITTED queries which are not handled by this patch.  These
issues need further evalution to determine what change, if any, is
needed.

Where the new error messages are generated, in most cases the best
fix will be to move code from the BEFORE trigger to an AFTER
trigger.  Where this is not feasible, the trigger can avoid the
error by re-issuing the triggering statement and returning NULL.

Documentation changes will be submitted in a separate patch.

Kevin Grittner and Tom Lane with input from Florian Pflug and
Robert Haas, based on problems encountered during conversion of
Wisconsin Circuit Court trigger logic to plpgsql triggers.

src/backend/access/heap/heapam.c

@@ -2352,27 +2352,26 @@ simple_heap_insert(Relation relation, HeapTuple tup)
  *
  *	relation - table to be modified (caller must hold suitable lock)
  *	tid - TID of tuple to be deleted
- *	ctid - output parameter, used only for failure case (see below)
- *	update_xmax - output parameter, used only for failure case (see below)
  *	cid - delete command ID (used for visibility test, and stored into
  *		cmax if successful)
  *	crosscheck - if not InvalidSnapshot, also check tuple against this
  *	wait - true if should wait for any conflicting update to commit/abort
+ *	hufd - output parameter, filled in failure cases (see below)
  *
  * Normal, successful return value is HeapTupleMayBeUpdated, which
  * actually means we did delete it.  Failure return codes are
  * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
  * (the last only possible if wait == false).
  *
- * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
- * If t_ctid is the same as tid, the tuple was deleted; if different, the
- * tuple was updated, and t_ctid is the location of the replacement tuple.
- * (t_xmax is needed to verify that the replacement tuple matches.)
+ * In the failure cases, the routine fills *hufd with the tuple's t_ctid,
+ * t_xmax, and t_cmax (the last only for HeapTupleSelfUpdated, since we
+ * cannot obtain cmax from a combocid generated by another transaction).
+ * See comments for struct HeapUpdateFailureData for additional info.
  */
 HTSU_Result
 heap_delete(Relation relation, ItemPointer tid,
-			ItemPointer ctid, TransactionId *update_xmax,
-			CommandId cid, Snapshot crosscheck, bool wait)
+			CommandId cid, Snapshot crosscheck, bool wait,
+			HeapUpdateFailureData *hufd)
 {
 	HTSU_Result result;
 	TransactionId xid = GetCurrentTransactionId();
@@ -2533,8 +2532,12 @@ l1:
 			   result == HeapTupleUpdated ||
 			   result == HeapTupleBeingUpdated);
 		Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID));
-		*ctid = tp.t_data->t_ctid;
-		*update_xmax = HeapTupleHeaderGetXmax(tp.t_data);
+		hufd->ctid = tp.t_data->t_ctid;
+		hufd->xmax = HeapTupleHeaderGetXmax(tp.t_data);
+		if (result == HeapTupleSelfUpdated)
+			hufd->cmax = HeapTupleHeaderGetCmax(tp.t_data);
+		else
+			hufd->cmax = 0;		/* for lack of an InvalidCommandId value */
 		UnlockReleaseBuffer(buffer);
 		if (have_tuple_lock)
 			UnlockTuple(relation, &(tp.t_self), ExclusiveLock);
@@ -2666,13 +2669,12 @@ void
 simple_heap_delete(Relation relation, ItemPointer tid)
 {
 	HTSU_Result result;
-	ItemPointerData update_ctid;
-	TransactionId update_xmax;
+	HeapUpdateFailureData hufd;
 
 	result = heap_delete(relation, tid,
-						 &update_ctid, &update_xmax,
 						 GetCurrentCommandId(true), InvalidSnapshot,
-						 true /* wait for commit */ );
+						 true /* wait for commit */,
+						 &hufd);
 	switch (result)
 	{
 		case HeapTupleSelfUpdated:
@@ -2703,12 +2705,11 @@ simple_heap_delete(Relation relation, ItemPointer tid)
  *	relation - table to be modified (caller must hold suitable lock)
  *	otid - TID of old tuple to be replaced
  *	newtup - newly constructed tuple data to store
- *	ctid - output parameter, used only for failure case (see below)
- *	update_xmax - output parameter, used only for failure case (see below)
  *	cid - update command ID (used for visibility test, and stored into
  *		cmax/cmin if successful)
  *	crosscheck - if not InvalidSnapshot, also check old tuple against this
  *	wait - true if should wait for any conflicting update to commit/abort
+ *	hufd - output parameter, filled in failure cases (see below)
  *
  * Normal, successful return value is HeapTupleMayBeUpdated, which
  * actually means we *did* update it.  Failure return codes are
@@ -2721,15 +2722,15 @@ simple_heap_delete(Relation relation, ItemPointer tid)
  * update was done.  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
- * tuple was updated, and t_ctid is the location of the replacement tuple.
- * (t_xmax is needed to verify that the replacement tuple matches.)
+ * In the failure cases, the routine fills *hufd with the tuple's t_ctid,
+ * t_xmax, and t_cmax (the last only for HeapTupleSelfUpdated, since we
+ * cannot obtain cmax from a combocid generated by another transaction).
+ * See comments for struct HeapUpdateFailureData for additional info.
  */
 HTSU_Result
 heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
-			ItemPointer ctid, TransactionId *update_xmax,
-			CommandId cid, Snapshot crosscheck, bool wait)
+			CommandId cid, Snapshot crosscheck, bool wait,
+			HeapUpdateFailureData *hufd)
 {
 	HTSU_Result result;
 	TransactionId xid = GetCurrentTransactionId();
@@ -2908,8 +2909,12 @@ l2:
 			   result == HeapTupleUpdated ||
 			   result == HeapTupleBeingUpdated);
 		Assert(!(oldtup.t_data->t_infomask & HEAP_XMAX_INVALID));
-		*ctid = oldtup.t_data->t_ctid;
-		*update_xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
+		hufd->ctid = oldtup.t_data->t_ctid;
+		hufd->xmax = HeapTupleHeaderGetXmax(oldtup.t_data);
+		if (result == HeapTupleSelfUpdated)
+			hufd->cmax = HeapTupleHeaderGetCmax(oldtup.t_data);
+		else
+			hufd->cmax = 0;		/* for lack of an InvalidCommandId value */
 		UnlockReleaseBuffer(buffer);
 		if (have_tuple_lock)
 			UnlockTuple(relation, &(oldtup.t_self), ExclusiveLock);
@@ -3379,13 +3384,12 @@ void
 simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
 {
 	HTSU_Result result;
-	ItemPointerData update_ctid;
-	TransactionId update_xmax;
+	HeapUpdateFailureData hufd;
 
 	result = heap_update(relation, otid, tup,
-						 &update_ctid, &update_xmax,
 						 GetCurrentCommandId(true), InvalidSnapshot,
-						 true /* wait for commit */ );
+						 true /* wait for commit */,
+						 &hufd);
 	switch (result)
 	{
 		case HeapTupleSelfUpdated:
@@ -3423,18 +3427,17 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
  * Output parameters:
  *	*tuple: all fields filled in
  *	*buffer: set to buffer holding tuple (pinned but not locked at exit)
- *	*ctid: set to tuple's t_ctid, but only in failure cases
- *	*update_xmax: set to tuple's xmax, but only in failure cases
+ *	*hufd: filled in failure cases (see below)
  *
  * Function result may be:
  *	HeapTupleMayBeUpdated: lock was successfully acquired
  *	HeapTupleSelfUpdated: lock failed because tuple updated by self
  *	HeapTupleUpdated: lock failed because tuple updated by other xact
  *
- * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
- * If t_ctid is the same as t_self, the tuple was deleted; if different, the
- * tuple was updated, and t_ctid is the location of the replacement tuple.
- * (t_xmax is needed to verify that the replacement tuple matches.)
+ * In the failure cases, the routine fills *hufd with the tuple's t_ctid,
+ * t_xmax, and t_cmax (the last only for HeapTupleSelfUpdated, since we
+ * cannot obtain cmax from a combocid generated by another transaction).
+ * See comments for struct HeapUpdateFailureData for additional info.
  *
  *
  * NOTES: because the shared-memory lock table is of finite size, but users
@@ -3470,9 +3473,9 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
  * conflict for a tuple, we don't incur any extra overhead.
  */
 HTSU_Result
-heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer,
-				ItemPointer ctid, TransactionId *update_xmax,
-				CommandId cid, LockTupleMode mode, bool nowait)
+heap_lock_tuple(Relation relation, HeapTuple tuple,
+				CommandId cid, LockTupleMode mode, bool nowait,
+				Buffer *buffer, HeapUpdateFailureData *hufd)
 {
 	HTSU_Result result;
 	ItemPointer tid = &(tuple->t_self);
@@ -3657,8 +3660,12 @@ l3:
 	{
 		Assert(result == HeapTupleSelfUpdated || result == HeapTupleUpdated);
 		Assert(!(tuple->t_data->t_infomask & HEAP_XMAX_INVALID));
-		*ctid = tuple->t_data->t_ctid;
-		*update_xmax = HeapTupleHeaderGetXmax(tuple->t_data);
+		hufd->ctid = tuple->t_data->t_ctid;
+		hufd->xmax = HeapTupleHeaderGetXmax(tuple->t_data);
+		if (result == HeapTupleSelfUpdated)
+			hufd->cmax = HeapTupleHeaderGetCmax(tuple->t_data);
+		else
+			hufd->cmax = 0;		/* for lack of an InvalidCommandId value */
 		LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
 		if (have_tuple_lock)
 			UnlockTuple(relation, tid, tuple_lock_type);

src/backend/commands/trigger.c

@@ -2571,8 +2571,7 @@ GetTupleForTrigger(EState *estate,
 	if (newSlot != NULL)
 	{
 		HTSU_Result test;
-		ItemPointerData update_ctid;
-		TransactionId update_xmax;
+		HeapUpdateFailureData hufd;
 
 		*newSlot = NULL;
 
@@ -2584,13 +2583,27 @@ GetTupleForTrigger(EState *estate,
 		 */
 ltrmark:;
 		tuple.t_self = *tid;
-		test = heap_lock_tuple(relation, &tuple, &buffer,
-							   &update_ctid, &update_xmax,
+		test = heap_lock_tuple(relation, &tuple,
 							   estate->es_output_cid,
-							   LockTupleExclusive, false);
+							   LockTupleExclusive, false /* wait */,
+							   &buffer, &hufd);
 		switch (test)
 		{
 			case HeapTupleSelfUpdated:
+				/*
+				 * The target tuple was already updated or deleted by the
+				 * current command, or by a later command in the current
+				 * transaction.  We ignore the tuple in the former case, and
+				 * throw error in the latter case, for the same reasons
+				 * enumerated in ExecUpdate and ExecDelete in
+				 * nodeModifyTable.c.
+				 */
+				if (hufd.cmax != estate->es_output_cid)
+					ereport(ERROR,
+							(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+							 errmsg("tuple to be updated was already modified by an operation triggered by the current command"),
+							 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
 				/* treat it as deleted; do not process */
 				ReleaseBuffer(buffer);
 				return NULL;
@@ -2604,7 +2617,7 @@ ltrmark:;
 					ereport(ERROR,
 							(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 							 errmsg("could not serialize access due to concurrent update")));
-				if (!ItemPointerEquals(&update_ctid, &tuple.t_self))
+				if (!ItemPointerEquals(&hufd.ctid, &tuple.t_self))
 				{
 					/* it was updated, so look at the updated version */
 					TupleTableSlot *epqslot;
@@ -2613,11 +2626,11 @@ ltrmark:;
 										   epqstate,
 										   relation,
 										   relinfo->ri_RangeTableIndex,
-										   &update_ctid,
-										   update_xmax);
+										   &hufd.ctid,
+										   hufd.xmax);
 					if (!TupIsNull(epqslot))
 					{
-						*tid = update_ctid;
+						*tid = hufd.ctid;
 						*newSlot = epqslot;
 
 						/*

src/backend/executor/execMain.c

@@ -1802,8 +1802,7 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
 		if (heap_fetch(relation, &SnapshotDirty, &tuple, &buffer, true, NULL))
 		{
 			HTSU_Result test;
-			ItemPointerData update_ctid;
-			TransactionId update_xmax;
+			HeapUpdateFailureData hufd;
 
 			/*
 			 * If xmin isn't what we're expecting, the slot must have been
@@ -1838,13 +1837,13 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
 			/*
 			 * If tuple was inserted by our own transaction, we have to check
 			 * cmin against es_output_cid: cmin >= current CID means our
-			 * command cannot see the tuple, so we should ignore it.  Without
-			 * this we are open to the "Halloween problem" of indefinitely
-			 * re-updating the same tuple. (We need not check cmax because
-			 * HeapTupleSatisfiesDirty will consider a tuple deleted by our
-			 * transaction dead, regardless of cmax.)  We just checked that
-			 * priorXmax == xmin, so we can test that variable instead of
-			 * doing HeapTupleHeaderGetXmin again.
+			 * command cannot see the tuple, so we should ignore it.
+			 * Otherwise heap_lock_tuple() will throw an error, and so would
+			 * any later attempt to update or delete the tuple.  (We need not
+			 * check cmax because HeapTupleSatisfiesDirty will consider a
+			 * tuple deleted by our transaction dead, regardless of cmax.)
+			 * Wee just checked that priorXmax == xmin, so we can test that
+			 * variable instead of doing HeapTupleHeaderGetXmin again.
 			 */
 			if (TransactionIdIsCurrentTransactionId(priorXmax) &&
 				HeapTupleHeaderGetCmin(tuple.t_data) >= estate->es_output_cid)
@@ -1856,17 +1855,29 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
 			/*
 			 * This is a live tuple, so now try to lock it.
 			 */
-			test = heap_lock_tuple(relation, &tuple, &buffer,
-								   &update_ctid, &update_xmax,
+			test = heap_lock_tuple(relation, &tuple,
 								   estate->es_output_cid,
-								   lockmode, false);
+								   lockmode, false /* wait */,
+								   &buffer, &hufd);
 			/* We now have two pins on the buffer, get rid of one */
 			ReleaseBuffer(buffer);
 
 			switch (test)
 			{
 				case HeapTupleSelfUpdated:
-					/* treat it as deleted; do not process */
+					/*
+					 * The target tuple was already updated or deleted by the
+					 * current command, or by a later command in the current
+					 * transaction.  We *must* ignore the tuple in the former
+					 * case, so as to avoid the "Halloween problem" of
+					 * repeated update attempts.  In the latter case it might
+					 * be sensible to fetch the updated tuple instead, but
+					 * doing so would require changing heap_lock_tuple as well
+					 * as heap_update and heap_delete to not complain about
+					 * updating "invisible" tuples, which seems pretty scary.
+					 * So for now, treat the tuple as deleted and do not
+					 * process.
+					 */
 					ReleaseBuffer(buffer);
 					return NULL;
 
@@ -1880,12 +1891,12 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
 						ereport(ERROR,
 								(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 								 errmsg("could not serialize access due to concurrent update")));
-					if (!ItemPointerEquals(&update_ctid, &tuple.t_self))
+					if (!ItemPointerEquals(&hufd.ctid, &tuple.t_self))
 					{
 						/* it was updated, so look at the updated version */
-						tuple.t_self = update_ctid;
+						tuple.t_self = hufd.ctid;
 						/* updated row should have xmin matching this xmax */
-						priorXmax = update_xmax;
+						priorXmax = hufd.xmax;
 						continue;
 					}
 					/* tuple was deleted, so give up */

src/backend/executor/nodeLockRows.c

@@ -71,8 +71,7 @@ lnext:
 		bool		isNull;
 		HeapTupleData tuple;
 		Buffer		buffer;
-		ItemPointerData update_ctid;
-		TransactionId update_xmax;
+		HeapUpdateFailureData hufd;
 		LockTupleMode lockmode;
 		HTSU_Result test;
 		HeapTuple	copyTuple;
@@ -117,15 +116,26 @@ lnext:
 		else
 			lockmode = LockTupleShared;
 
-		test = heap_lock_tuple(erm->relation, &tuple, &buffer,
-							   &update_ctid, &update_xmax,
+		test = heap_lock_tuple(erm->relation, &tuple,
 							   estate->es_output_cid,
-							   lockmode, erm->noWait);
+							   lockmode, erm->noWait,
+							   &buffer, &hufd);
 		ReleaseBuffer(buffer);
 		switch (test)
 		{
 			case HeapTupleSelfUpdated:
-				/* treat it as deleted; do not process */
+				/*
+				 * The target tuple was already updated or deleted by the
+				 * current command, or by a later command in the current
+				 * transaction.  We *must* ignore the tuple in the former
+				 * case, so as to avoid the "Halloween problem" of repeated
+				 * update attempts.  In the latter case it might be sensible
+				 * to fetch the updated tuple instead, but doing so would
+				 * require changing heap_lock_tuple as well as heap_update and
+				 * heap_delete to not complain about updating "invisible"
+				 * tuples, which seems pretty scary.  So for now, treat the
+				 * tuple as deleted and do not process.
+				 */
 				goto lnext;
 
 			case HeapTupleMayBeUpdated:
@@ -137,8 +147,7 @@ lnext:
 					ereport(ERROR,
 							(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 							 errmsg("could not serialize access due to concurrent update")));
-				if (ItemPointerEquals(&update_ctid,
-									  &tuple.t_self))
+				if (ItemPointerEquals(&hufd.ctid, &tuple.t_self))
 				{
 					/* Tuple was deleted, so don't return it */
 					goto lnext;
@@ -146,7 +155,7 @@ lnext:
 
 				/* updated, so fetch and lock the updated version */
 				copyTuple = EvalPlanQualFetch(estate, erm->relation, lockmode,
-											  &update_ctid, update_xmax);
+											  &hufd.ctid, hufd.xmax);
 
 				if (copyTuple == NULL)
 				{

src/backend/executor/nodeModifyTable.c

@@ -295,8 +295,7 @@ ExecDelete(ItemPointer tupleid,
 	ResultRelInfo *resultRelInfo;
 	Relation	resultRelationDesc;
 	HTSU_Result result;
-	ItemPointerData update_ctid;
-	TransactionId update_xmax;
+	HeapUpdateFailureData hufd;
 
 	/*
 	 * get information on the (current) result relation
@@ -348,14 +347,44 @@ ExecDelete(ItemPointer tupleid,
 		 */
 ldelete:;
 		result = heap_delete(resultRelationDesc, tupleid,
-							 &update_ctid, &update_xmax,
 							 estate->es_output_cid,
 							 estate->es_crosscheck_snapshot,
-							 true /* wait for commit */ );
+							 true /* wait for commit */,
+							 &hufd);
 		switch (result)
 		{
 			case HeapTupleSelfUpdated:
-				/* already deleted by self; nothing to do */
+				/*
+				 * The target tuple was already updated or deleted by the
+				 * current command, or by a later command in the current
+				 * transaction.  The former case is possible in a join DELETE
+				 * where multiple tuples join to the same target tuple.
+				 * This is somewhat questionable, but Postgres has always
+				 * allowed it: we just ignore additional deletion attempts.
+				 *
+				 * The latter case arises if the tuple is modified by a
+				 * command in a BEFORE trigger, or perhaps by a command in a
+				 * volatile function used in the query.  In such situations we
+				 * should not ignore the deletion, but it is equally unsafe to
+				 * proceed.  We don't want to discard the original DELETE
+				 * while keeping the triggered actions based on its deletion;
+				 * and it would be no better to allow the original DELETE
+				 * while discarding updates that it triggered.  The row update
+				 * carries some information that might be important according
+				 * to business rules; so throwing an error is the only safe
+				 * course.
+				 *
+				 * If a trigger actually intends this type of interaction,
+				 * it can re-execute the DELETE and then return NULL to
+				 * cancel the outer delete.
+				 */
+				if (hufd.cmax != estate->es_output_cid)
+					ereport(ERROR,
+							(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+							 errmsg("tuple to be updated was already modified by an operation triggered by the current command"),
+							 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
+				/* Else, already deleted by self; nothing to do */
 				return NULL;
 
 			case HeapTupleMayBeUpdated:
@@ -366,7 +395,7 @@ ldelete:;
 					ereport(ERROR,
 							(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 							 errmsg("could not serialize access due to concurrent update")));
-				if (!ItemPointerEquals(tupleid, &update_ctid))
+				if (!ItemPointerEquals(tupleid, &hufd.ctid))
 				{
 					TupleTableSlot *epqslot;
 
@@ -374,11 +403,11 @@ ldelete:;
 										   epqstate,
 										   resultRelationDesc,
 										   resultRelInfo->ri_RangeTableIndex,
-										   &update_ctid,
-										   update_xmax);
+										   &hufd.ctid,
+										   hufd.xmax);
 					if (!TupIsNull(epqslot))
 					{
-						*tupleid = update_ctid;
+						*tupleid = hufd.ctid;
 						goto ldelete;
 					}
 				}
@@ -482,8 +511,7 @@ ExecUpdate(ItemPointer tupleid,
 	ResultRelInfo *resultRelInfo;
 	Relation	resultRelationDesc;
 	HTSU_Result result;
-	ItemPointerData update_ctid;
-	TransactionId update_xmax;
+	HeapUpdateFailureData hufd;
 	List	   *recheckIndexes = NIL;
 
 	/*
@@ -564,14 +592,43 @@ lreplace:;
 		 * mode transactions.
 		 */
 		result = heap_update(resultRelationDesc, tupleid, tuple,
-							 &update_ctid, &update_xmax,
 							 estate->es_output_cid,
 							 estate->es_crosscheck_snapshot,
-							 true /* wait for commit */ );
+							 true /* wait for commit */,
+							 &hufd);
 		switch (result)
 		{
 			case HeapTupleSelfUpdated:
-				/* already deleted by self; nothing to do */
+				/*
+				 * The target tuple was already updated or deleted by the
+				 * current command, or by a later command in the current
+				 * transaction.  The former case is possible in a join UPDATE
+				 * where multiple tuples join to the same target tuple.
+				 * This is pretty questionable, but Postgres has always
+				 * allowed it: we just execute the first update action and
+				 * ignore additional update attempts.
+				 *
+				 * The latter case arises if the tuple is modified by a
+				 * command in a BEFORE trigger, or perhaps by a command in a
+				 * volatile function used in the query.  In such situations we
+				 * should not ignore the update, but it is equally unsafe to
+				 * proceed.  We don't want to discard the original UPDATE
+				 * while keeping the triggered actions based on it; and we
+				 * have no principled way to merge this update with the
+				 * previous ones.  So throwing an error is the only safe
+				 * course.
+				 *
+				 * If a trigger actually intends this type of interaction,
+				 * it can re-execute the UPDATE (assuming it can figure out
+				 * how) and then return NULL to cancel the outer update.
+				 */
+				if (hufd.cmax != estate->es_output_cid)
+					ereport(ERROR,
+							(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+							 errmsg("tuple to be updated was already modified by an operation triggered by the current command"),
+							 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
+				/* Else, already updated by self; nothing to do */
 				return NULL;
 
 			case HeapTupleMayBeUpdated:
@@ -582,7 +639,7 @@ lreplace:;
 					ereport(ERROR,
 							(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
 							 errmsg("could not serialize access due to concurrent update")));
-				if (!ItemPointerEquals(tupleid, &update_ctid))
+				if (!ItemPointerEquals(tupleid, &hufd.ctid))
 				{
 					TupleTableSlot *epqslot;
 
@@ -590,11 +647,11 @@ lreplace:;
 										   epqstate,
 										   resultRelationDesc,
 										   resultRelInfo->ri_RangeTableIndex,
-										   &update_ctid,
-										   update_xmax);
+										   &hufd.ctid,
+										   hufd.xmax);
 					if (!TupIsNull(epqslot))
 					{
-						*tupleid = update_ctid;
+						*tupleid = hufd.ctid;
 						slot = ExecFilterJunk(resultRelInfo->ri_junkFilter, epqslot);
 						tuple = ExecMaterializeSlot(slot);
 						goto lreplace;

src/include/access/heapam.h

@@ -35,6 +35,29 @@ typedef enum
 	LockTupleExclusive
 } LockTupleMode;
 
+/*
+ * When heap_update, heap_delete, or heap_lock_tuple fail because the target
+ * tuple is already outdated, they fill in this struct to provide information
+ * to the caller about what happened.
+ * ctid is the target's ctid link: it is the same as the target's TID if the
+ * target was deleted, or the location of the replacement tuple if the target
+ * was updated.
+ * xmax is the outdating transaction's XID.  If the caller wants to visit the
+ * replacement tuple, it must check that this matches before believing the
+ * replacement is really a match.
+ * cmax is the outdating command's CID, but only when the failure code is
+ * HeapTupleSelfUpdated (i.e., something in the current transaction outdated
+ * the tuple); otherwise cmax is zero.  (We make this restriction because
+ * HeapTupleHeaderGetCmax doesn't work for tuples outdated in other
+ * transactions.)
+ */
+typedef struct HeapUpdateFailureData
+{
+	ItemPointerData		ctid;
+	TransactionId		xmax;
+	CommandId			cmax;
+} HeapUpdateFailureData;
+
 
 /* ----------------
  *		function prototypes for heap access method
@@ -100,16 +123,15 @@ extern Oid heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 extern void heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 				  CommandId cid, int options, BulkInsertState bistate);
 extern HTSU_Result heap_delete(Relation relation, ItemPointer tid,
-			ItemPointer ctid, TransactionId *update_xmax,
-			CommandId cid, Snapshot crosscheck, bool wait);
+			CommandId cid, Snapshot crosscheck, bool wait,
+			HeapUpdateFailureData *hufd);
 extern HTSU_Result heap_update(Relation relation, ItemPointer otid,
 			HeapTuple newtup,
-			ItemPointer ctid, TransactionId *update_xmax,
-			CommandId cid, Snapshot crosscheck, bool wait);
+			CommandId cid, Snapshot crosscheck, bool wait,
+			HeapUpdateFailureData *hufd);
 extern HTSU_Result heap_lock_tuple(Relation relation, HeapTuple tuple,
-				Buffer *buffer, ItemPointer ctid,
-				TransactionId *update_xmax, CommandId cid,
-				LockTupleMode mode, bool nowait);
+				CommandId cid, LockTupleMode mode, bool nowait,
+				Buffer *buffer, HeapUpdateFailureData *hufd);
 extern void heap_inplace_update(Relation relation, HeapTuple tuple);
 extern bool heap_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid);
 extern bool heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid,

src/test/regress/expected/triggers.out

@@ -1550,3 +1550,197 @@ drop table depth_a, depth_b, depth_c;
 drop function depth_a_tf();
 drop function depth_b_tf();
 drop function depth_c_tf();
+--
+-- Test updates to rows during firing of BEFORE ROW triggers.
+-- As of 9.2, such cases should be rejected (see bug #6123).
+--
+create temp table parent (
+    aid int not null primary key,
+    val1 text,
+    val2 text,
+    val3 text,
+    val4 text,
+    bcnt int not null default 0);
+create temp table child (
+    bid int not null primary key,
+    aid int not null,
+    val1 text);
+create function parent_upd_func()
+  returns trigger language plpgsql as
+$$
+begin
+  if old.val1 <> new.val1 then
+    new.val2 = new.val1;
+    delete from child where child.aid = new.aid and child.val1 = new.val1;
+  end if;
+  return new;
+end;
+$$;
+create trigger parent_upd_trig before update on parent
+  for each row execute procedure parent_upd_func();
+create function parent_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  delete from child where aid = old.aid;
+  return old;
+end;
+$$;
+create trigger parent_del_trig before delete on parent
+  for each row execute procedure parent_del_func();
+create function child_ins_func()
+  returns trigger language plpgsql as
+$$
+begin
+  update parent set bcnt = bcnt + 1 where aid = new.aid;
+  return new;
+end;
+$$;
+create trigger child_ins_trig after insert on child
+  for each row execute procedure child_ins_func();
+create function child_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  update parent set bcnt = bcnt - 1 where aid = old.aid;
+  return old;
+end;
+$$;
+create trigger child_del_trig after delete on child
+  for each row execute procedure child_del_func();
+insert into parent values (1, 'a', 'a', 'a', 'a', 0);
+insert into child values (10, 1, 'b');
+select * from parent; select * from child;
+ aid | val1 | val2 | val3 | val4 | bcnt 
+-----+------+------+------+------+------
+   1 | a    | a    | a    | a    |    1
+(1 row)
+
+ bid | aid | val1 
+-----+-----+------
+  10 |   1 | b
+(1 row)
+
+update parent set val1 = 'b' where aid = 1; -- should fail
+ERROR:  tuple to be updated was already modified by an operation triggered by the current command
+HINT:  Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.
+select * from parent; select * from child;
+ aid | val1 | val2 | val3 | val4 | bcnt 
+-----+------+------+------+------+------
+   1 | a    | a    | a    | a    |    1
+(1 row)
+
+ bid | aid | val1 
+-----+-----+------
+  10 |   1 | b
+(1 row)
+
+delete from parent where aid = 1; -- should fail
+ERROR:  tuple to be updated was already modified by an operation triggered by the current command
+HINT:  Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.
+select * from parent; select * from child;
+ aid | val1 | val2 | val3 | val4 | bcnt 
+-----+------+------+------+------+------
+   1 | a    | a    | a    | a    |    1
+(1 row)
+
+ bid | aid | val1 
+-----+-----+------
+  10 |   1 | b
+(1 row)
+
+-- replace the trigger function with one that restarts the deletion after
+-- having modified a child
+create or replace function parent_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  delete from child where aid = old.aid;
+  if found then
+    delete from parent where aid = old.aid;
+    return null; -- cancel outer deletion
+  end if;
+  return old;
+end;
+$$;
+delete from parent where aid = 1;
+select * from parent; select * from child;
+ aid | val1 | val2 | val3 | val4 | bcnt 
+-----+------+------+------+------+------
+(0 rows)
+
+ bid | aid | val1 
+-----+-----+------
+(0 rows)
+
+drop table parent, child;
+drop function parent_upd_func();
+drop function parent_del_func();
+drop function child_ins_func();
+drop function child_del_func();
+-- similar case, but with a self-referencing FK so that parent and child
+-- rows can be affected by a single operation
+create temp table self_ref_trigger (
+    id int primary key,
+    parent int references self_ref_trigger,
+    data text,
+    nchildren int not null default 0
+);
+create function self_ref_trigger_ins_func()
+  returns trigger language plpgsql as
+$$
+begin
+  if new.parent is not null then
+    update self_ref_trigger set nchildren = nchildren + 1
+      where id = new.parent;
+  end if;
+  return new;
+end;
+$$;
+create trigger self_ref_trigger_ins_trig before insert on self_ref_trigger
+  for each row execute procedure self_ref_trigger_ins_func();
+create function self_ref_trigger_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  if old.parent is not null then
+    update self_ref_trigger set nchildren = nchildren - 1
+      where id = old.parent;
+  end if;
+  return old;
+end;
+$$;
+create trigger self_ref_trigger_del_trig before delete on self_ref_trigger
+  for each row execute procedure self_ref_trigger_del_func();
+insert into self_ref_trigger values (1, null, 'root');
+insert into self_ref_trigger values (2, 1, 'root child A');
+insert into self_ref_trigger values (3, 1, 'root child B');
+insert into self_ref_trigger values (4, 2, 'grandchild 1');
+insert into self_ref_trigger values (5, 3, 'grandchild 2');
+update self_ref_trigger set data = 'root!' where id = 1;
+select * from self_ref_trigger;
+ id | parent |     data     | nchildren 
+----+--------+--------------+-----------
+  2 |      1 | root child A |         1
+  4 |      2 | grandchild 1 |         0
+  3 |      1 | root child B |         1
+  5 |      3 | grandchild 2 |         0
+  1 |        | root!        |         2
+(5 rows)
+
+delete from self_ref_trigger;
+ERROR:  tuple to be updated was already modified by an operation triggered by the current command
+HINT:  Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.
+select * from self_ref_trigger;
+ id | parent |     data     | nchildren 
+----+--------+--------------+-----------
+  2 |      1 | root child A |         1
+  4 |      2 | grandchild 1 |         0
+  3 |      1 | root child B |         1
+  5 |      3 | grandchild 2 |         0
+  1 |        | root!        |         2
+(5 rows)
+
+drop table self_ref_trigger;
+drop function self_ref_trigger_ins_func();
+drop function self_ref_trigger_del_func();

src/test/regress/sql/triggers.sql

@@ -1025,3 +1025,158 @@ drop table depth_a, depth_b, depth_c;
 drop function depth_a_tf();
 drop function depth_b_tf();
 drop function depth_c_tf();
+
+--
+-- Test updates to rows during firing of BEFORE ROW triggers.
+-- As of 9.2, such cases should be rejected (see bug #6123).
+--
+
+create temp table parent (
+    aid int not null primary key,
+    val1 text,
+    val2 text,
+    val3 text,
+    val4 text,
+    bcnt int not null default 0);
+create temp table child (
+    bid int not null primary key,
+    aid int not null,
+    val1 text);
+
+create function parent_upd_func()
+  returns trigger language plpgsql as
+$$
+begin
+  if old.val1 <> new.val1 then
+    new.val2 = new.val1;
+    delete from child where child.aid = new.aid and child.val1 = new.val1;
+  end if;
+  return new;
+end;
+$$;
+create trigger parent_upd_trig before update on parent
+  for each row execute procedure parent_upd_func();
+
+create function parent_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  delete from child where aid = old.aid;
+  return old;
+end;
+$$;
+create trigger parent_del_trig before delete on parent
+  for each row execute procedure parent_del_func();
+
+create function child_ins_func()
+  returns trigger language plpgsql as
+$$
+begin
+  update parent set bcnt = bcnt + 1 where aid = new.aid;
+  return new;
+end;
+$$;
+create trigger child_ins_trig after insert on child
+  for each row execute procedure child_ins_func();
+
+create function child_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  update parent set bcnt = bcnt - 1 where aid = old.aid;
+  return old;
+end;
+$$;
+create trigger child_del_trig after delete on child
+  for each row execute procedure child_del_func();
+
+insert into parent values (1, 'a', 'a', 'a', 'a', 0);
+insert into child values (10, 1, 'b');
+select * from parent; select * from child;
+
+update parent set val1 = 'b' where aid = 1; -- should fail
+select * from parent; select * from child;
+
+delete from parent where aid = 1; -- should fail
+select * from parent; select * from child;
+
+-- replace the trigger function with one that restarts the deletion after
+-- having modified a child
+create or replace function parent_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  delete from child where aid = old.aid;
+  if found then
+    delete from parent where aid = old.aid;
+    return null; -- cancel outer deletion
+  end if;
+  return old;
+end;
+$$;
+
+delete from parent where aid = 1;
+select * from parent; select * from child;
+
+drop table parent, child;
+
+drop function parent_upd_func();
+drop function parent_del_func();
+drop function child_ins_func();
+drop function child_del_func();
+
+-- similar case, but with a self-referencing FK so that parent and child
+-- rows can be affected by a single operation
+
+create temp table self_ref_trigger (
+    id int primary key,
+    parent int references self_ref_trigger,
+    data text,
+    nchildren int not null default 0
+);
+
+create function self_ref_trigger_ins_func()
+  returns trigger language plpgsql as
+$$
+begin
+  if new.parent is not null then
+    update self_ref_trigger set nchildren = nchildren + 1
+      where id = new.parent;
+  end if;
+  return new;
+end;
+$$;
+create trigger self_ref_trigger_ins_trig before insert on self_ref_trigger
+  for each row execute procedure self_ref_trigger_ins_func();
+
+create function self_ref_trigger_del_func()
+  returns trigger language plpgsql as
+$$
+begin
+  if old.parent is not null then
+    update self_ref_trigger set nchildren = nchildren - 1
+      where id = old.parent;
+  end if;
+  return old;
+end;
+$$;
+create trigger self_ref_trigger_del_trig before delete on self_ref_trigger
+  for each row execute procedure self_ref_trigger_del_func();
+
+insert into self_ref_trigger values (1, null, 'root');
+insert into self_ref_trigger values (2, 1, 'root child A');
+insert into self_ref_trigger values (3, 1, 'root child B');
+insert into self_ref_trigger values (4, 2, 'grandchild 1');
+insert into self_ref_trigger values (5, 3, 'grandchild 2');
+
+update self_ref_trigger set data = 'root!' where id = 1;
+
+select * from self_ref_trigger;
+
+delete from self_ref_trigger;
+
+select * from self_ref_trigger;
+
+drop table self_ref_trigger;
+drop function self_ref_trigger_ins_func();
+drop function self_ref_trigger_del_func();