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

Andres Freund <andres(at)anarazel(dot)de> writes:
>> 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.

Yup, it absolutely is. This idea of having the expression compiler just
reorder the tlist entries is definitely interesting. I recall that I
wondered about whether we could do that when I first made the MULTIEXPR
patch, but doing it in the parse tree causes a lot of problems because
there are places that assume resjunk entries come after not-resjunk ones.
I don't see a reason why we couldn't reorder during compile though ---
and that will work in all the branches we still support.

The main concern I've got about this prototype is that it's not clear
to me whether we can back-patch addition of a new EEOP step type without
causing ABI issues. However, why do we need a new step type? Seems to
me that EEOP_SUBPLAN will serve fine, if we just undo the special
treatment of MULTIEXPR in ExecScanSubPlan and let it go ahead and
evaluate the subplan and assign param values.

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

My recollection is that the planner is pretty cavalier about whether
resjunk entries get marked as such in lower plan levels. I wouldn't
worry about this (but by the same token, don't do anything that
relies on the resjunk marks being accurate).

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

Agreed, that's nothing to be doing in a bug-fix patch. I think we just
want to re-order the steps to put the EEOP_SUBPLAN at the front of the
tlist, and then get rid of the execPlan manipulations and the other
special-casing of MULTIEXPR. Anything else would be HEAD-only.

Are you planning to push forward with this, or do you want me to?
It's really my bug, since the existing MULTIEXPR implementation
is my fault.

regards, tom lane