Re: BUG #18830: ExecInitMerge Segfault on MERGE

Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年3月5日周三 02:56写道：

> On Tue, 4 Mar 2025 at 09:30, David Rowley <dgrowleyml@gmail.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.
>

Yeah,  I agree with you. As I said before, this fix will get a wrong result
compared to
enable_partition_pruning = off, even though no crash happened again.
At least for NOT MATCH, we must have resultRelInfo entry.


> 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.


I have not tried this way.  I'm not sure about this.

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.
>

Yeah, this way changes a lot of codes.

-- 
Thanks,
Tender Wang