From a67a4a8a5cc3609dd57a496be5614adee2000b5f Mon Sep 17 00:00:00 2001
From: amitlan <amitlangote09@gmail.com>
Date: Tue, 31 Mar 2020 15:36:27 +0900
Subject: [PATCH v1] Build root tuple in ExecBuildSlotValueDescription

Having the logic to do so be spread across various callers is
starting to seem cumbersome to maintain.

Use mt_root_tuple_slot to store the tuple to be shown in the
description. For it to be available in all cases that require it,
always set it in ExecInitModifyTable instead if only when UPDATE
tuple routing is active as is the case currently.
---
 src/backend/executor/execMain.c        | 204 ++++++++-------------------------
 src/backend/executor/execPartition.c   |   6 +
 src/backend/executor/nodeModifyTable.c |  21 ++--
 src/include/nodes/execnodes.h          |   6 +
 src/test/regress/expected/inherit.out  |   2 +-
 src/test/regress/expected/update.out   |   6 +-
 6 files changed, 80 insertions(+), 165 deletions(-)

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 4fdffad..0b295c0 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -93,11 +93,10 @@ static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
 									  Bitmapset *modifiedCols,
 									  AclMode requiredPerms);
 static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
-static char *ExecBuildSlotValueDescription(Oid reloid,
-										   TupleTableSlot *slot,
-										   TupleDesc tupdesc,
-										   Bitmapset *modifiedCols,
-										   int maxfieldlen);
+static char *ExecBuildSlotValueDescription(EState *estate,
+							  ResultRelInfo *resultRelInfo,
+							  TupleTableSlot *slot,
+							  int maxfieldlen);
 static void EvalPlanQualStart(EPQState *epqstate, Plan *planTree);
 
 /*
@@ -1829,51 +1828,10 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
 							TupleTableSlot *slot,
 							EState *estate)
 {
-	Oid			root_relid;
-	TupleDesc	tupdesc;
 	char	   *val_desc;
-	Bitmapset  *modifiedCols;
 
-	/*
-	 * 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 in the error message
-	 * matches the input tuple.
-	 */
-	if (resultRelInfo->ri_PartitionRoot)
-	{
-		TupleDesc	old_tupdesc;
-		AttrMap    *map;
-
-		root_relid = RelationGetRelid(resultRelInfo->ri_PartitionRoot);
-		tupdesc = RelationGetDescr(resultRelInfo->ri_PartitionRoot);
-
-		old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
-		/* a reverse map */
-		map = build_attrmap_by_name_if_req(old_tupdesc, tupdesc);
-
-		/*
-		 * Partition-specific slot's tupdesc can't be changed, so allocate a
-		 * new one.
-		 */
-		if (map != NULL)
-			slot = execute_attr_map_slot(map, slot,
-										 MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
-	}
-	else
-	{
-		root_relid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
-		tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
-	}
-
-	modifiedCols = bms_union(GetInsertedColumns(resultRelInfo, estate),
-							 GetUpdatedColumns(resultRelInfo, estate));
-
-	val_desc = ExecBuildSlotValueDescription(root_relid,
-											 slot,
-											 tupdesc,
-											 modifiedCols,
-											 64);
+	val_desc = ExecBuildSlotValueDescription(estate, resultRelInfo,
+											 slot, 64);
 	ereport(ERROR,
 			(errcode(ERRCODE_CHECK_VIOLATION),
 			 errmsg("new row for relation \"%s\" violates partition constraint",
@@ -1900,9 +1858,6 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 	Relation	rel = resultRelInfo->ri_RelationDesc;
 	TupleDesc	tupdesc = RelationGetDescr(rel);
 	TupleConstr *constr = tupdesc->constr;
-	Bitmapset  *modifiedCols;
-	Bitmapset  *insertedCols;
-	Bitmapset  *updatedCols;
 
 	Assert(constr || resultRelInfo->ri_PartitionCheck);
 
@@ -1918,51 +1873,19 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 			if (att->attnotnull && slot_attisnull(slot, attrChk))
 			{
 				char	   *val_desc;
-				Relation	orig_rel = rel;
-				TupleDesc	orig_tupdesc = RelationGetDescr(rel);
-
-				/*
-				 * 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)
-				{
-					AttrMap    *map;
-
-					rel = resultRelInfo->ri_PartitionRoot;
-					tupdesc = RelationGetDescr(rel);
-					/* a reverse map */
-					map = build_attrmap_by_name_if_req(orig_tupdesc,
-													   tupdesc);
 
-					/*
-					 * Partition-specific slot's tupdesc can't be changed, so
-					 * allocate a new one.
-					 */
-					if (map != NULL)
-						slot = execute_attr_map_slot(map, slot,
-													 MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
-				}
-
-				insertedCols = GetInsertedColumns(resultRelInfo, estate);
-				updatedCols = GetUpdatedColumns(resultRelInfo, estate);
-				modifiedCols = bms_union(insertedCols, updatedCols);
-				val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
+				val_desc = ExecBuildSlotValueDescription(estate,
+														 resultRelInfo,
 														 slot,
-														 tupdesc,
-														 modifiedCols,
 														 64);
 
 				ereport(ERROR,
 						(errcode(ERRCODE_NOT_NULL_VIOLATION),
 						 errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint",
 								NameStr(att->attname),
-								RelationGetRelationName(orig_rel)),
+								RelationGetRelationName(rel)),
 						 val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
-						 errtablecol(orig_rel, attrChk)));
+						 errtablecol(rel, attrChk)));
 			}
 		}
 	}
@@ -1974,43 +1897,17 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
 		if ((failed = ExecRelCheck(resultRelInfo, slot, estate)) != NULL)
 		{
 			char	   *val_desc;
-			Relation	orig_rel = rel;
 
-			/* See the comment above. */
-			if (resultRelInfo->ri_PartitionRoot)
-			{
-				TupleDesc	old_tupdesc = RelationGetDescr(rel);
-				AttrMap    *map;
-
-				rel = resultRelInfo->ri_PartitionRoot;
-				tupdesc = RelationGetDescr(rel);
-				/* a reverse map */
-				map = build_attrmap_by_name_if_req(old_tupdesc,
-												   tupdesc);
-
-				/*
-				 * Partition-specific slot's tupdesc can't be changed, so
-				 * allocate a new one.
-				 */
-				if (map != NULL)
-					slot = execute_attr_map_slot(map, slot,
-												 MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
-			}
-
-			insertedCols = GetInsertedColumns(resultRelInfo, estate);
-			updatedCols = GetUpdatedColumns(resultRelInfo, estate);
-			modifiedCols = bms_union(insertedCols, updatedCols);
-			val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
+			val_desc = ExecBuildSlotValueDescription(estate,
+													 resultRelInfo,
 													 slot,
-													 tupdesc,
-													 modifiedCols,
 													 64);
 			ereport(ERROR,
 					(errcode(ERRCODE_CHECK_VIOLATION),
 					 errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
-							RelationGetRelationName(orig_rel), failed),
+							RelationGetRelationName(rel), failed),
 					 val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
-					 errtableconstraint(orig_rel, failed)));
+					 errtableconstraint(rel, failed)));
 		}
 	}
 }
@@ -2028,8 +1925,6 @@ void
 ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
 					 TupleTableSlot *slot, EState *estate)
 {
-	Relation	rel = resultRelInfo->ri_RelationDesc;
-	TupleDesc	tupdesc = RelationGetDescr(rel);
 	ExprContext *econtext;
 	ListCell   *l1,
 			   *l2;
@@ -2067,9 +1962,6 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
 		if (!ExecQual(wcoExpr, econtext))
 		{
 			char	   *val_desc;
-			Bitmapset  *modifiedCols;
-			Bitmapset  *insertedCols;
-			Bitmapset  *updatedCols;
 
 			switch (wco->kind)
 			{
@@ -2083,34 +1975,9 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
 					 * USING policy.
 					 */
 				case WCO_VIEW_CHECK:
-					/* See the comment in ExecConstraints(). */
-					if (resultRelInfo->ri_PartitionRoot)
-					{
-						TupleDesc	old_tupdesc = RelationGetDescr(rel);
-						AttrMap    *map;
-
-						rel = resultRelInfo->ri_PartitionRoot;
-						tupdesc = RelationGetDescr(rel);
-						/* a reverse map */
-						map = build_attrmap_by_name_if_req(old_tupdesc,
-														   tupdesc);
-
-						/*
-						 * Partition-specific slot's tupdesc can't be changed,
-						 * so allocate a new one.
-						 */
-						if (map != NULL)
-							slot = execute_attr_map_slot(map, slot,
-														 MakeTupleTableSlot(tupdesc, &TTSOpsVirtual));
-					}
-
-					insertedCols = GetInsertedColumns(resultRelInfo, estate);
-					updatedCols = GetUpdatedColumns(resultRelInfo, estate);
-					modifiedCols = bms_union(insertedCols, updatedCols);
-					val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel),
+					val_desc = ExecBuildSlotValueDescription(estate,
+															 resultRelInfo,
 															 slot,
-															 tupdesc,
-															 modifiedCols,
 															 64);
 
 					ereport(ERROR,
@@ -2163,8 +2030,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
  *
  * Also, unlike the case with index entries, we need to be prepared to ignore
  * dropped columns.  We used to use the slot's tuple descriptor to decode the
- * data, but the slot's descriptor doesn't identify dropped columns, so we
- * now need to be passed the relation's descriptor.
+ * data, but the slot's descriptor doesn't identify dropped columns.
  *
  * Note that, like BuildIndexValueDescription, if the user does not have
  * permission to view any of the columns involved, a NULL is returned.  Unlike
@@ -2173,10 +2039,9 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
  * columns they are.
  */
 static char *
-ExecBuildSlotValueDescription(Oid reloid,
+ExecBuildSlotValueDescription(EState *estate,
+							  ResultRelInfo *resultRelInfo,
 							  TupleTableSlot *slot,
-							  TupleDesc tupdesc,
-							  Bitmapset *modifiedCols,
 							  int maxfieldlen)
 {
 	StringInfoData buf;
@@ -2187,6 +2052,9 @@ ExecBuildSlotValueDescription(Oid reloid,
 	AclResult	aclresult;
 	bool		table_perm = false;
 	bool		any_perm = false;
+	Oid			reloid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
+	TupleDesc	tupdesc;
+	Bitmapset  *modifiedCols;
 
 	/*
 	 * Check if RLS is enabled and should be active for the relation; if so,
@@ -2196,6 +2064,34 @@ ExecBuildSlotValueDescription(Oid reloid,
 	if (check_enable_rls(reloid, InvalidOid, true) == RLS_ENABLED)
 		return NULL;
 
+	/*
+	 * 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 matches the input
+	 * tuple.
+	 */
+	if (resultRelInfo->ri_PartitionRootSlot)
+	{
+		TupleDesc	old_tupdesc;
+		AttrMap    *map;
+
+		tupdesc = resultRelInfo->ri_PartitionRootSlot->tts_tupleDescriptor;
+
+		old_tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
+		/* a reverse map */
+		map = build_attrmap_by_name_if_req(old_tupdesc, tupdesc);
+
+		/* Note that this uses a new slot for the converted tuple. */
+		if (map != NULL)
+			slot = execute_attr_map_slot(map, slot,
+										 resultRelInfo->ri_PartitionRootSlot);
+	}
+	else
+		tupdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
+
+	modifiedCols = bms_union(GetInsertedColumns(resultRelInfo, estate),
+							 GetUpdatedColumns(resultRelInfo, estate));
+
 	initStringInfo(&buf);
 
 	appendStringInfoChar(&buf, '(');
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index fb6ce49..9069c5b 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -527,6 +527,12 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
 					  estate->es_instrument);
 
 	/*
+	 * ri_PartitionRootSlot will be used when converting partition's tuples to
+	 * root format.
+	 */
+	leaf_part_rri->ri_PartitionRootSlot = mtstate->mt_root_tuple_slot;
+
+	/*
 	 * Verify result relation is a valid target for an INSERT.  An UPDATE of a
 	 * partition-key becomes a DELETE+INSERT operation, so this check is still
 	 * required when the operation is CMD_UPDATE.
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index d71c0a4..9369ee7 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2320,8 +2320,14 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 
 	/* If modifying a partitioned table, initialize the root table info */
 	if (node->rootResultRelIndex >= 0)
+	{
+		Relation	rootrel;
+
 		mtstate->rootResultRelInfo = estate->es_root_result_relations +
 			node->rootResultRelIndex;
+		rootrel = mtstate->rootResultRelInfo->ri_RelationDesc;
+		mtstate->mt_root_tuple_slot = table_slot_create(rootrel, NULL);
+	}
 
 	mtstate->mt_arowmarks = (List **) palloc0(sizeof(List *) * nplans);
 	mtstate->mt_nplans = nplans;
@@ -2403,6 +2409,12 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 															 eflags);
 		}
 
+		/*
+		 * ri_PartitionRootSlot will be used when converting partition's
+		 * tuples to root format.
+		 */
+		resultRelInfo->ri_PartitionRootSlot = mtstate->mt_root_tuple_slot;
+
 		resultRelInfo++;
 		i++;
 	}
@@ -2445,10 +2457,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
 	 * We can skip this setup if it's not a partition key update.
 	 */
 	if (update_tuple_routing_needed)
-	{
 		ExecSetupChildParentMapForSubplan(mtstate);
-		mtstate->mt_root_tuple_slot = table_slot_create(rel, NULL);
-	}
 
 	/*
 	 * Initialize any WITH CHECK OPTION constraints if needed.
@@ -2774,12 +2783,10 @@ ExecEndModifyTable(ModifyTableState *node)
 	 * and release the slot used for tuple routing, if set.
 	 */
 	if (node->mt_partition_tuple_routing)
-	{
 		ExecCleanupTupleRouting(node, node->mt_partition_tuple_routing);
 
-		if (node->mt_root_tuple_slot)
-			ExecDropSingleTupleTableSlot(node->mt_root_tuple_slot);
-	}
+	if (node->mt_root_tuple_slot)
+		ExecDropSingleTupleTableSlot(node->mt_root_tuple_slot);
 
 	/*
 	 * Free the exprcontext
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 0fb5d61..fbcff6a 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -486,6 +486,12 @@ typedef struct ResultRelInfo
 	/* relation descriptor for root partitioned table */
 	Relation	ri_PartitionRoot;
 
+	/*
+	 * Slot initialized with root table descriptor; may be set even if
+	 * ri_PartitionRoot is not.
+	 */
+	TupleTableSlot *ri_PartitionRootSlot;
+
 	/* Additional information specific to partition tuple routing */
 	struct PartitionRoutingInfo *ri_PartitionInfo;
 
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 2b68aef..fbfff7b 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2494,7 +2494,7 @@ ERROR:  new row for relation "errtst_child_plaindef" violates check constraint "
 DETAIL:  Failing row contains (10, 1, 15).
 UPDATE errtst_parent SET data = data + 10 WHERE partid = 20;
 ERROR:  new row for relation "errtst_child_reorder" violates check constraint "errtst_child_reorder_data_check"
-DETAIL:  Failing row contains (15, 1, 20).
+DETAIL:  Failing row contains (20, 1, 15).
 -- direct leaf partition update, without partition id violation
 BEGIN;
 UPDATE errtst_child_fastdef SET partid = 1 WHERE partid = 0;
diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out
index b7f90de..91a8280 100644
--- a/src/test/regress/expected/update.out
+++ b/src/test/regress/expected/update.out
@@ -344,7 +344,7 @@ DETAIL:  Failing row contains (105, 85, null, b, 15).
 -- but "a = 'a'" violates partition constraint enforced by root partition)
 UPDATE part_b_10_b_20 set a = 'a';
 ERROR:  new row for relation "part_c_1_100" violates partition constraint
-DETAIL:  Failing row contains (null, 1, 96, 12, a).
+DETAIL:  Failing row contains (null, 96, a, 12, 1).
 -- ok, partition key update, no constraint violation
 UPDATE range_parted set d = d - 10 WHERE d > 10;
 -- ok, no partition key update, no constraint violation
@@ -375,7 +375,7 @@ UPDATE part_b_10_b_20 set c = c + 20 returning c, b, a;
 -- fail, row movement happens only within the partition subtree.
 UPDATE part_b_10_b_20 set b = b - 6 WHERE c > 116 returning *;
 ERROR:  new row for relation "part_d_1_15" violates partition constraint
-DETAIL:  Failing row contains (2, 117, 2, b, 7).
+DETAIL:  Failing row contains (2, 117, b, 7, 2).
 -- ok, row movement, with subset of rows moved into different partition.
 UPDATE range_parted set b = b - 6 WHERE c > 116 returning a, b + c;
  a | ?column? 
@@ -817,7 +817,7 @@ INSERT into sub_parted VALUES (1,2,10);
 -- constraint is inherited from upper root.
 UPDATE sub_parted set a = 2 WHERE c = 10;
 ERROR:  new row for relation "sub_part2" violates partition constraint
-DETAIL:  Failing row contains (2, 10, 2).
+DETAIL:  Failing row contains (2, 2, 10).
 -- Test update-partition-key, where the unpruned partitions do not have their
 -- partition keys updated.
 SELECT tableoid::regclass::text, * FROM list_parted WHERE a = 2 ORDER BY 1;
-- 
1.8.3.1

