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

Hi,

On 2023-02-24 12:26:06 -0500, Tom Lane wrote:
> 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.

Yea, I had briefly looked at what it would take to reorder in the planner, and
quickly gave up.

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

I think we could introduce a new step type, but I also agree we can easily
work around needing that. The main reason I didn't use EEOP_SUBPLAN was that
it seemed cleaner to not assume that there's a return value / a place to put a
pseudo return value. But we could easily make that a variant of EEOP_SUBPLAN
in the back branches.

One argument for a separate step type / separate signature for evaluating a
MULTIEXPR is that that will make it easier to evaluate arguments as part of
the surrounding ExprState.

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

Makes sense.

I noticed this because I'd initially put in an a defense assert ensuring that
we'd not see a MULTIEXPR in a non-resjunk tle, which triggered in the pushdown
case.

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

I'd happy if you had a go at it. I might take a stab at moving the the
argument evaluation inline, after this goes in.

The amount of "mini-expressions" is one of the main sources of overhead with
JIT. Which also got worse over time with more and more partitioning related
stuff...

Greetings,

Andres Freund