Re: ModifyTable overheads in generic plans

I'm still a bit confused and unhappy about the initialization of 
ResultRelInfos and the various fields in them. We've made progress in 
the previous patches, but it's still a bit messy.

> 		/*
> 		 * If transition tuples will be captured, initialize a map to convert
> 		 * child tuples into the format of the table mentioned in the query
> 		 * (root relation), because the transition tuple store can only store
> 		 * tuples in the root table format.  However for INSERT, the map is
> 		 * only initialized for a given partition when the partition itself is
> 		 * first initialized by ExecFindPartition.  Also, this map is also
> 		 * needed if an UPDATE ends up having to move tuples across
> 		 * partitions, because in that case the child tuple to be moved first
> 		 * needs to be converted into the root table's format.  In that case,
> 		 * we use GetChildToRootMap() to either create one from scratch if
> 		 * we didn't already create it here.
> 		 *
> 		 * Note: We cannot always initialize this map lazily, that is, use
> 		 * GetChildToRootMap(), because AfterTriggerSaveEvent(), which needs
> 		 * the map, doesn't have access to the "target" relation that is
> 		 * needed to create the map.
> 		 */
> 		if (mtstate->mt_transition_capture && operation != CMD_INSERT)
> 		{
> 			Relation	relation = resultRelInfo->ri_RelationDesc;
> 			Relation	targetRel = mtstate->rootResultRelInfo->ri_RelationDesc;
> 
> 			resultRelInfo->ri_ChildToRootMap =
> 				convert_tuples_by_name(RelationGetDescr(relation),
> 									   RelationGetDescr(targetRel));
> 			/* First time creating the map for this result relation. */
> 			Assert(!resultRelInfo->ri_ChildToRootMapValid);
> 			resultRelInfo->ri_ChildToRootMapValid = true;
> 		}

The comment explains that AfterTriggerSaveEvent() cannot use 
GetChildToRootMap(), because it doesn't have access to the root target 
relation. But there is a field for that in ResultRelInfo: 
ri_PartitionRoot. However, that's only set up when we do partition routing.

How about we rename ri_PartitionRoot to e.g ri_RootTarget, and set it 
always, even for non-partition inheritance? We have that information 
available when we initialize the ResultRelInfo, so might as well.

Some code currently checks ri_PartitionRoot, to determine if a tuple 
that's been inserted, has been routed. For example:

> 		/*
> 		 * Also check the tuple against the partition constraint, if there is
> 		 * one; except that if we got here via tuple-routing, we don't need to
> 		 * if there's no BR trigger defined on the partition.
> 		 */
> 		if (resultRelationDesc->rd_rel->relispartition &&
> 			(resultRelInfo->ri_PartitionRoot == NULL ||
> 			 (resultRelInfo->ri_TrigDesc &&
> 			  resultRelInfo->ri_TrigDesc->trig_insert_before_row)))
> 			ExecPartitionCheck(resultRelInfo, slot, estate, true);

So if we set ri_PartitionRoot always, we would need some other way to 
determine if the tuple at hand has actually been routed or not. But 
wouldn't that be a good thing anyway? Isn't it possible that the same 
ResultRelInfo is sometimes used for routed tuples, and sometimes for 
tuples that have been inserted/updated "directly"? 
ExecLookupUpdateResultRelByOid() sets that field lazily, so I think it 
would be possible to get here with ri_PartitionRoot either set or not, 
depending on whether an earlier cross-partition update was routed to the 
table.

The above check is just an optimization, to skip unnecessary 
ExecPartitionCheck() calls, but I think this snippet in 
ExecConstraints() needs to get this right:

> 				/*
> 				 * 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));
> 				}

Is that an existing bug, or am I missing?

- Heikki