From e408234633c01817d6a2313fdbdccdb4f0057c1e Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Fri, 6 Jan 2017 15:53:10 +0900
Subject: [PATCH 1/7] Fix reporting of violation in ExecConstraints, again

We decided in f1b4c771ea74f42447dccaed42ffcdcccf3aa694 that passing
the original slot (one containing the tuple formatted per root
partitioned table's tupdesc) to ExecConstraints(), but that breaks
certain cases.  Imagine what would happen if a BR trigger changed the
tuple - the original slot would not contain those changes.
So, it seems better to convert (if necessary) the tuple formatted
per partition tupdesc after tuple-routing back to the root table's
format and use the converted tuple to make val_desc shown in the
message if an error occurs.
---
 src/backend/commands/copy.c            |  6 ++--
 src/backend/executor/execMain.c        | 53 +++++++++++++++++++++++++++++-----
 src/backend/executor/nodeModifyTable.c |  5 ++--
 src/include/executor/executor.h        |  3 +-
 src/test/regress/expected/insert.out   | 18 ++++++++++--
 src/test/regress/sql/insert.sql        | 17 ++++++++++-
 6 files changed, 82 insertions(+), 20 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f56b2ac49b..65eb167087 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2491,8 +2491,7 @@ CopyFrom(CopyState cstate)
 
 	for (;;)
 	{
-		TupleTableSlot *slot,
-					   *oldslot;
+		TupleTableSlot *slot;
 		bool		skip_tuple;
 		Oid			loaded_oid = InvalidOid;
 
@@ -2534,7 +2533,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;
@@ -2625,7 +2623,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 ff277d300a..332c54c819 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1763,8 +1763,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot,
  */
 void
 ExecConstraints(ResultRelInfo *resultRelInfo,
-				TupleTableSlot *slot, TupleTableSlot *orig_slot,
-				EState *estate)
+				TupleTableSlot *slot, EState *estate)
 {
 	Relation	rel = resultRelInfo->ri_RelationDesc;
 	TupleDesc	tupdesc = RelationGetDescr(rel);
@@ -1787,23 +1786,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
+				 * In case where the tuple is 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
+				 * type 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);
@@ -1830,15 +1843,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);
@@ -1860,15 +1885,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/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 4692427e60..23e04893b8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -262,7 +262,6 @@ ExecInsert(ModifyTableState *mtstate,
 	Relation	resultRelationDesc;
 	Oid			newId;
 	List	   *recheckIndexes = NIL;
-	TupleTableSlot *oldslot = slot;
 
 	/*
 	 * get the heap tuple out of the tuple table slot, making sure we have a
@@ -433,7 +432,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)
 		{
@@ -994,7 +993,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 b9c7f72903..3e8d64686e 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -195,8 +195,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 ca3134c34c..501d50eeac 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -335,10 +335,24 @@ select tableoid::regclass, * from p;
 
 truncate p;
 alter table p add constraint check_b check (b = 3);
+create function p11_trig_fn()
+returns trigger AS
+$$
+begin
+  NEW.b := 4;
+  return NEW;
+end;
+$$
+language plpgsql;
+create trigger p11_trig before insert ON p11
+  for each row execute procedure p11_trig_fn();
 -- check that correct input row is shown when constraint check_b fails on p11
--- after "(1, 2)" is routed to it
+-- after "(1, 2)" is routed to it (actually "(1, 4)" would be shown due to the
+-- BR trigger p11_trig_fn)
 insert into p values (1, 2);
 ERROR:  new row for relation "p11" violates check constraint "check_b"
-DETAIL:  Failing row contains (1, 2).
+DETAIL:  Failing row contains (1, 4).
+drop trigger p11_trig on p11;
+drop function p11_trig_fn();
 -- cleanup
 drop table p, p1, p11;
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 09c9879da1..22aa94e181 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -197,9 +197,24 @@ select tableoid::regclass, * from p;
 
 truncate p;
 alter table p add constraint check_b check (b = 3);
+create function p11_trig_fn()
+returns trigger AS
+$$
+begin
+  NEW.b := 4;
+  return NEW;
+end;
+$$
+language plpgsql;
+create trigger p11_trig before insert ON p11
+  for each row execute procedure p11_trig_fn();
+
 -- check that correct input row is shown when constraint check_b fails on p11
--- after "(1, 2)" is routed to it
+-- after "(1, 2)" is routed to it (actually "(1, 4)" would be shown due to the
+-- BR trigger p11_trig_fn)
 insert into p values (1, 2);
+drop trigger p11_trig on p11;
+drop function p11_trig_fn();
 
 -- cleanup
 drop table p, p1, p11;
-- 
2.11.0

