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

Hi,

On 2023-02-21 15:55:15 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2023-02-21 15:16:11 -0500, Tom Lane wrote:
> >> It occurs to me that one possible fix is to make MULTIEXPR_SUBLINK
> >> and the associated output Params use a separate ParamExecData array;
> >> instead of the query-wide es_param_exec_vals array, use one that
> >> is local to the specific targetlist's ExprState. I'm not sure how
> >> much violence that does to the current notion of an ExprState ---
> >> do we think that is read-only during execution?
>
> > I don't think you would need to modify ExprState - the information about
> > params etc comes from the ExprContext, right? So we'd need to build a
> > different ExprContext for partitions, and use that when evaluating the
> > expressions.
> > ...
> > We don't currently have infrastructure for setting
> > econtext->ecxt_param_exec_vals to something else, but that shouldn't be too
> > hard to add.
>
> No, that won't work, because many usages of PARAM_EXEC Params are
> specifically intended to transmit datums from one expression (plan node)
> to another. That's why that array was query-global to begin with.
> What I'm wondering about is adding a separate array, and likely a separate
> ParamKind, that would have a less-than-query-wide scope. We might be able
> to get away with having that be plan-node-wide, but making it local to the
> specific compiled expression feels safer and easier to reason about.

What I was trying to suggest is that you could have a dedicated ExprContext
that'd point to such a separate array. That'd allow you to choose the the
separate array on a per-expression-evaluation basis (not even per ExprState).
We already have multiple ExprContexts in some nodes, so this wouldn't break
new ground.

Greetings,

Andres Freund