Re: BUG #18830: ExecInitMerge Segfault on MERGE

On Wed, Mar 5, 2025 at 3:56 AM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> 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.

Thanks -- I’ve studied the code and I agree with the conclusion: when
all result relations are pruned, we still need to lock and process the
first one to preserve executor invariants.

Your examples with ExecMerge() and ExecInitPartitionInfo() make the
consequences of missing resultRelInfo[0] pretty clear. Locking and
including the first result relation, even if pruned, seems like the
most straightforward way to maintain correctness without deeper
structural changes.

I've come up with the attached. I'll still need to add a test case.

--
Thanks, Amit Langote