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

Hi,

On 2023-02-21 15:16:11 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Afaict this is a problem of a wrongly generated target list, which isn't what
> > ExecInterpExprStillValid() guards against:
>
> The targetlists are okay, really. The core problem is that each
> targetlist has an instance of the MULTIEXPR_SUBLINK SubPlan with a
> differently-mutated "args" list, and it looks to me like we correctly
> mutated that for the associated child table. But because all the
> instances share the same output Params, the ExecSetParamPlan mechanism
> gets confused about which version of the SubPlan it ought to invoke
> to recompute the output Params.

It doesn't seem crazy to describe that as a wrong targetlist :). But anyway,
all I meant was that the problem isn't one that ExecInterpExprStillValid() can
handle.

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

Oh, I guess you might be referencing ExecInitExprWithParams(), from
6719b238e8f0? I don't like that much, it seems like the wrong level - all
other similar info comes from the ExprContext, why not here as well? Either
way, it looks to me like it'd not be used here, as that's just used for
PARAM_EXTERN, but we're dealing with PARAM_EXEC. Just changing the
->ext_params at runtime wouldn't work, it seems to be resolved when the
expression is built.

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.

> If we did have a local-to-the-expression ParamExecData array, maybe that
> could be used to get a cleaner fix for things like the domain VALUES
> and case-test-expression hacks.

Hm, I'm not quite following along here.

Greetings,

Andres Freund