Re: MERGE issues around inheritance

On Mon, May 26, 2025 at 4:11 AM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>
> On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> >
>
> 2. ExecInitModifyTable() does not initialize the WCO lists or
> RETURNING list for rootResultRelInfo, so those never get executed.
>
> As it happens, it is possible to construct cases where (1) causes a
> crash, even without WCO's or a RETURNING list (see the first test case
> in the attached patch), so this bug goes all the way back to v15,
> where MERGE was introduced.
>
> So I think we need to do something like the attached.
>
> There is perhaps scope to reduce the code duplication between this and
> ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
> think it's best to leave that for now.

+ Relation rootRelation = rootRelInfo->ri_RelationDesc;
+ Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc;
+ int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;

firstResultRel may equal (==) to rootRelation,
in that case, we don't need to call map_variable_attnos?

we can
returningList = linitial(node->returningLists);
if (rel != firstResultRel)
{
/* Convert any Vars in it to contain the root's attno's */
part_attmap =
build_attrmap_by_name(RelationGetDescr(rel),
RelationGetDescr(firstResultRel),
false);
returningList = (List *)
map_variable_attnos((Node *) returningList,
firstVarno, 0,
part_attmap,
RelationGetForm(rel)->reltype,
&found_whole_row);
}
(i am not sure that will make code less readable).

we can unconditionally call ExecBuildProjectionInfo for rootRelInfo
within ExecInitModifyTable instead of in ExecInitMerge.
right after

/*
* Build a projection for each result rel.
*/
resultRelInfo = mtstate->resultRelInfo;
foreach(l, returningLists)
{
List *rlist = (List *) lfirst(l);
resultRelInfo->ri_returningList = rlist;
resultRelInfo->ri_projectReturning =
ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
resultRelInfo->ri_RelationDesc->rd_att);
resultRelInfo++;
}

This would make related initiation logic stay together, also harmless (i think).
what do you think?