Fix reporting of violations in ExecConstraints, again.

We decided in f1b4c771ea74f42447dccaed42ffcdcccf3aa694 to pass the
original slot to ExecConstraints(), but that breaks when there are
BEFORE ROW triggers involved.  So we need to do reverse-map the tuples
back to the original descriptor instead, as Amit originally proposed.

Amit Langote, reviewed by Ashutosh Bapat.  One overlooked comment
fixed by me.

Discussion: http://postgr.es/m/b3a17254-6849-e542-2353-bde4e880b6a4@lab.ntt.co.jp

7 files changed, 92 insertions, 28 deletions

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 8c588086863..73677be59e0 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2506,8 +2506,7 @@ CopyFrom(CopyState cstate)
 
 	for (;;)
 	{
-		TupleTableSlot *slot,
-				   *oldslot;
+		TupleTableSlot *slot;
 		bool		skip_tuple;
 		Oid			loaded_oid = InvalidOid;
 
@@ -2549,7 +2548,6 @@ CopyFrom(CopyState cstate)
 		ExecStoreTuple(tuple, slot, InvalidBuffer, false);
 
 		/* Determine the partition to heap_insert the tuple into */
-		oldslot = slot;
 		if (cstate->partition_dispatch_info)
 		{
 			int			leaf_part_index;
@@ -2651,7 +2649,7 @@ CopyFrom(CopyState cstate)
 				/* Check the constraints of the tuple */
 				if (cstate->rel->rd_att->constr ||
 					resultRelInfo->ri_PartitionCheck)
-					ExecConstraints(resultRelInfo, slot, oldslot, estate);
+					ExecConstraints(resultRelInfo, slot, estate);
 
 				if (useHeapMultiInsert)
 				{
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 920b12072fb..5c12fb457d5 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1824,14 +1824,12 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
  * the partition constraint, if any.
  *
  * Note: 'slot' contains the tuple to check the constraints of, which may
- * have been converted from the original input tuple after tuple routing,
- * while 'orig_slot' contains the original tuple to be shown in the message,
- * if an error occurs.
+ * have been converted from the original input tuple after tuple routing.
+ * 'resultRelInfo' is the original result relation, before tuple routing.
  */
 void
 ExecConstraints(ResultRelInfo *resultRelInfo,
-				TupleTableSlot *slot, TupleTableSlot *orig_slot,
-				EState *estate)
+				TupleTableSlot *slot, EState *estate)
 {
 	Relation	rel = resultRelInfo->ri_RelationDesc;
 	TupleDesc	tupdesc = RelationGetDescr(rel);
@@ -1854,23 +1852,37 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 			{
 				char	   *val_desc;
 				Relation	orig_rel = rel;
-				TupleDesc	orig_tupdesc = tupdesc;
+				TupleDesc	orig_tupdesc = RelationGetDescr(rel);
 
 				/*
-				 * choose the correct relation to build val_desc from the
-				 * tuple contained in orig_slot
+				 * If the tuple has been routed, it's been converted to the
+				 * partition's rowtype, which might differ from the root
+				 * table's.  We must convert it back to the root table's
+				 * rowtype so that val_desc shown error message matches the
+				 * input tuple.
 				 */
 				if (resultRelInfo->ri_PartitionRoot)
 				{
+					HeapTuple	tuple = ExecFetchSlotTuple(slot);
+					TupleConversionMap	*map;
+
 					rel = resultRelInfo->ri_PartitionRoot;
 					tupdesc = RelationGetDescr(rel);
+					/* a reverse map */
+					map = convert_tuples_by_name(orig_tupdesc, tupdesc,
+								gettext_noop("could not convert row type"));
+					if (map != NULL)
+					{
+						tuple = do_convert_tuple(tuple, map);
+						ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+					}
 				}
 
 				insertedCols = GetInsertedColumns(resultRelInfo, estate);
 				updatedCols = GetUpdatedColumns(resultRelInfo, estate);
 				modifiedCols = bms_union(insertedCols, updatedCols);
 				val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
-														 orig_slot,
+														 slot,
 														 tupdesc,
 														 modifiedCols,
 														 64);
@@ -1897,15 +1909,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 			/* See the comment above. */
 			if (resultRelInfo->ri_PartitionRoot)
 			{
+				HeapTuple	tuple = ExecFetchSlotTuple(slot);
+				TupleDesc	old_tupdesc = RelationGetDescr(rel);
+				TupleConversionMap	*map;
+
 				rel = resultRelInfo->ri_PartitionRoot;
 				tupdesc = RelationGetDescr(rel);
+				/* a reverse map */
+				map = convert_tuples_by_name(old_tupdesc, tupdesc,
+							gettext_noop("could not convert row type"));
+				if (map != NULL)
+				{
+					tuple = do_convert_tuple(tuple, map);
+					ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+				}
 			}
 
 			insertedCols = GetInsertedColumns(resultRelInfo, estate);
 			updatedCols = GetUpdatedColumns(resultRelInfo, estate);
 			modifiedCols = bms_union(insertedCols, updatedCols);
 			val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
-													 orig_slot,
+													 slot,
 													 tupdesc,
 													 modifiedCols,
 													 64);
@@ -1927,15 +1951,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 		/* See the comment above. */
 		if (resultRelInfo->ri_PartitionRoot)
 		{
+			HeapTuple	tuple = ExecFetchSlotTuple(slot);
+			TupleDesc	old_tupdesc = RelationGetDescr(rel);
+			TupleConversionMap	*map;
+
 			rel = resultRelInfo->ri_PartitionRoot;
 			tupdesc = RelationGetDescr(rel);
+			/* a reverse map */
+			map = convert_tuples_by_name(old_tupdesc, tupdesc,
+						gettext_noop("could not convert row type"));
+			if (map != NULL)
+			{
+				tuple = do_convert_tuple(tuple, map);
+				ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+			}
 		}
 
 		insertedCols = GetInsertedColumns(resultRelInfo, estate);
 		updatedCols = GetUpdatedColumns(resultRelInfo, estate);
 		modifiedCols = bms_union(insertedCols, updatedCols);
 		val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
-												 orig_slot,
+												 slot,
 												 tupdesc,
 												 modifiedCols,
 												 64);
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index f20d728ad5a..327a0bad388 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -389,7 +389,7 @@ ExecSimpleRelationInsert(EState *estate, TupleTableSlot *slot)
 
 		/* Check the constraints of the tuple */
 		if (rel->rd_att->constr)
-			ExecConstraints(resultRelInfo, slot, slot, estate);
+			ExecConstraints(resultRelInfo, slot, estate);
 
 		/* Store the slot into tuple that we can inspect. */
 		tuple = ExecMaterializeSlot(slot);
@@ -448,7 +448,7 @@ ExecSimpleRelationUpdate(EState *estate, EPQState *epqstate,
 
 		/* Check the constraints of the tuple */
 		if (rel->rd_att->constr)
-			ExecConstraints(resultRelInfo, slot, slot, estate);
+			ExecConstraints(resultRelInfo, slot, estate);
 
 		/* Store the slot into tuple that we can write. */
 		tuple = ExecMaterializeSlot(slot);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 0b524e0b7cf..00b736c22c4 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -263,8 +263,7 @@ ExecInsert(ModifyTableState *mtstate,
 	Relation	resultRelationDesc;
 	Oid			newId;
 	List	   *recheckIndexes = NIL;
-	TupleTableSlot *oldslot = slot,
-			   *result = NULL;
+	TupleTableSlot *result = NULL;
 
 	/*
 	 * get the heap tuple out of the tuple table slot, making sure we have a
@@ -435,7 +434,7 @@ ExecInsert(ModifyTableState *mtstate,
 		 * Check the constraints of the tuple
 		 */
 		if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
-			ExecConstraints(resultRelInfo, slot, oldslot, estate);
+			ExecConstraints(resultRelInfo, slot, estate);
 
 		if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0)
 		{
@@ -993,7 +992,7 @@ lreplace:;
 		 * tuple-routing is performed here, hence the slot remains unchanged.
 		 */
 		if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
-			ExecConstraints(resultRelInfo, slot, slot, estate);
+			ExecConstraints(resultRelInfo, slot, estate);
 
 		/*
 		 * replace the heap tuple
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index d3849b93eb1..f7f3189a1a0 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -186,8 +186,7 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
 extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
 extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
 extern void ExecConstraints(ResultRelInfo *resultRelInfo,
-				TupleTableSlot *slot, TupleTableSlot *orig_slot,
-				EState *estate);
+				TupleTableSlot *slot, EState *estate);
 extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
 					 TupleTableSlot *slot, EState *estate);
 extern LockTupleMode ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo);
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 7fafa982126..6f34b1c6406 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -354,11 +354,26 @@ ERROR:  no partition of relation "mlparted1" found for row
 DETAIL:  Partition key of the failing row contains ((b + 0)) = (5).
 truncate mlparted;
 alter table mlparted add constraint check_b check (b = 3);
--- check that correct input row is shown when constraint check_b fails on mlparted11
--- after "(1, 2)" is routed to it
+-- have a BR trigger modify the row such that the check_b is violated
+create function mlparted11_trig_fn()
+returns trigger AS
+$$
+begin
+  NEW.b := 4;
+  return NEW;
+end;
+$$
+language plpgsql;
+create trigger mlparted11_trig before insert ON mlparted11
+  for each row execute procedure mlparted11_trig_fn();
+-- check that the correct row is shown when constraint check_b fails after
+-- "(1, 2)" is routed to mlparted11 (actually "(1, 4)" would be shown due
+-- to the BR trigger mlparted11_trig_fn)
 insert into mlparted values (1, 2);
 ERROR:  new row for relation "mlparted11" violates check constraint "check_b"
-DETAIL:  Failing row contains (1, 2).
+DETAIL:  Failing row contains (1, 4).
+drop trigger mlparted11_trig on mlparted11;
+drop function mlparted11_trig_fn();
 -- check that inserting into an internal partition successfully results in
 -- checking its partition constraint before inserting into the leaf partition
 -- selected by tuple-routing
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index f9c00705a22..020854c960e 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -217,9 +217,26 @@ insert into mlparted (a, b) values (1, 5);
 
 truncate mlparted;
 alter table mlparted add constraint check_b check (b = 3);
--- check that correct input row is shown when constraint check_b fails on mlparted11
--- after "(1, 2)" is routed to it
+
+-- have a BR trigger modify the row such that the check_b is violated
+create function mlparted11_trig_fn()
+returns trigger AS
+$$
+begin
+  NEW.b := 4;
+  return NEW;
+end;
+$$
+language plpgsql;
+create trigger mlparted11_trig before insert ON mlparted11
+  for each row execute procedure mlparted11_trig_fn();
+
+-- check that the correct row is shown when constraint check_b fails after
+-- "(1, 2)" is routed to mlparted11 (actually "(1, 4)" would be shown due
+-- to the BR trigger mlparted11_trig_fn)
 insert into mlparted values (1, 2);
+drop trigger mlparted11_trig on mlparted11;
+drop function mlparted11_trig_fn();
 
 -- check that inserting into an internal partition successfully results in
 -- checking its partition constraint before inserting into the leaf partition