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

Hi,

On 2023-02-23 17:06:03 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I'm not sure I really like the design of the params being local to a single
> > ExprState, or even local to individual steps in the expression. It seems to
> > buy further into making MULTIEXPR a special case.
>
> Well, it *is* a special case, because it's (ab)using a mechanism
> originally meant only for initplans to do something else.

I guess my discomfort about per-expression (or really, per subplan) param
originates in that feeling foreign to the whole point of params, which is to
share a state between expressions. What you're proposing is a bespoke thing,
that only works within a single targetlist. With those restrictions it feels
like it's misusing PARAM_EXEC.

And we're not even really the per-subplan param arrays for different values,
just for different ParamExecData->execPlan fields.

> Maybe we should throw out the whole implementation and start over, but that
> line of thinking isn't going to lead to something back-patchable. I'm not
> very sure what we would do fundamentally differently anyway.

The way MULTIEXPR expressions work seems pretty weird to - partially inherited
from initPlans, admittedly.

My understanding is:

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.

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.

Greetings,

Andres Freund