Re: BUG #17800: ON CONFLICT DO UPDATE fails to detect incompatible fields that leads to a server crash

Hi,

On 2023-02-23 15:36:36 -0800, Andres Freund wrote:
> When we build the evaluation step for the Param, we don't yet know that we're
> dealing with a MULTIEXPR (nor do we have a reference to the relevant
> SubPlan)). At the end of the targetlist, we have a special SubPlan, which make
> ExecInitSubPlan() set ParamExecData->execPlan to its SubPlanState for all the
> output parameters, to let ExecEvalParamExec know that the first reference to
> one of the output params needs to evalute the plan. But that means that we
> need to reset execPlan between rows, which is handled by the no-output
> ExecScanSubPlan() invocation at the end of the targetlist. That just seems
> baroque.

There's at least one case in the regression tests where a correlated MULTIEXPR
is in a non-resjunk TLE. I assume due to subquery pushdown. Is there a
problem with that? I don't immediately see any, but though it's worth
mentioning.

>
> ISTM that a saner sequence of expression steps would be:
> - steps to evalute the 1st argument of the MULTIEXPR, targetting SubPlanState->args[0]
> - steps to evalute the 2nd argument of the MULTIEXPR, targetting SubPlanState->args[1]
> ...
> - step to execute the subplan, computing output parameters
> - PARAM_SUBPLAN step referencing one of the outputs
> - steps for another output column
> - PARAM_SUBPLAN step referencing one of the outputs
> ...
>
> That'd completely obviate the need for any use of execPlan and thus remove the
> problem with getting confused about which subplan we need to execute.
>
>
> Unfortunately, we can't easily produce that today, because we don't have easy
> access to the SubPlan[State] at the time we encounter the Params.
>
>
> I am starting to wonder if a cleaner fix wouldn't be to add magic to
> ExecBuildProjectionInfo(), to find the junklist targetlist with the subplan,
> and then generate something like what I described above. Likely skipping the
> optimized/inlined evaluation of the arguments, initially at least.
>
>
> I didn't think of this until just now, but we actually already do a separate
> traversal of the expressions: ExecInitExprSlots(). Obviously the name
> wouldn't fit anymore, but it seems perfectly suited for collecting subplans
> that we'd need to evaluate?
>
>
> Let me try to hack that up.

Here's a rough prototype for that. Certainly would need a good bit more
polish, but I think the approach looks pretty promising?

I didn't do the part about evaluating the 'input' parameters as part of the
outer ExprState. Still think that's a good idea, but it's somewhat orthogonal
to the problems we're trying to fix.

Greetings,

Andres Freund