Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Tue, 4 Mar 2025 at 09:30, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> It looks like this is happening because ExecInitModifyTable() skips
> adding mergeActionsList items when bms_is_member(rti,
> estate->es_unpruned_relids) is false for all resultRelations. This
> results in an empty actions list. Because the MERGE is performing a
> LEFT JOIN for the NOT MATCHED, ExecMerge() gets a row and runs
> ExecMergeNotMatched(), which crashes on "econtext->ecxt_scantuple =
> NULL;" because of a NULL econtext. econtext is NULL because
> ExecInitMerge() skips calling ExecAssignExprContext() when
> mergeActionLists is empty.
>
> There are a couple of ways I can see to fix this, 1) would be to move
> the ExecAssignExprContext() above the "if (mergeActionLists == NIL)"
> in ExecInitMerge(), or 2) add code to return NULL in
> ExecMergeNotMatched() if actionStates is NULL.

Hmm, I don't think that's right. I think this is just masking the problem.

I think the real problem is that, as things stand,
ExecInitModifyTable() must never be allowed to prune every leaf
partition, because there are multiple places that rely on there being
at least one resultRelInfo.

For example, ExecModifyTable() passes the first resultRelInfo to
ExecMerge() and ExecMergeNotMatched() in the NOT MATCHED case, and
those use the action lists in that resultRelInfo to work out what
action to execute. The fact that it didn't crash with this patch is
probably just luck, because it's passing a pointer to uninitialised
memory, but it's still doing the wrong thing by not invoking any merge
actions.

Similarly, if MERGE does an INSERT on a partitioned table, the code in
ExecInitPartitionInfo() from ExecFindPartition() assumes that
mtstate->resultRelInfo has at least one entry.

So I think the simplest solution is to arrange for
ExecInitModifyTable() to always include details of at least one result
relation, adding at least one entry to each of the lists. As an
alternative, it might be possible to make the executor cope with an
empty resultRelInfo array (by using the root resultRelInfo instead,
where one is needed), but I think that would be a bigger change.

Regards,
Dean