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

Hi,

On 2023-02-23 13:53:34 -0500, Tom Lane wrote:
> I wrote:
> > Hmm ... this doesn't look very much like what I was imagining. Let
> > me draft a prototype and we can compare.
>
> Here's what I was thinking about. I didn't bother adding regression
> test cases yet, but it fixes both of the symptoms Alexander found.

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. Particularly because we here
don't actually need multiple values to live at the same time, we just need
multiple execPlan fields.

Doing that amount of additional work in ExecReadyExpr() seems worrisome to me
- looks like it'd trigger in a lot of expressions that won't need any
adjustment. We could easily short-circuit based on last_param not being set
though.

But perhaps we don't actually need the work in ExecReadyExpr()? What if we
moved private_exec_vals + a bitmap when to use it into ExprState? Afaict we
don't have cases where single paramid could be used multiple times within a
single expression?

I think that might also provide a better basis for redesigning CaseTestExpr
etc, they could use that ExprState local param array as well?

Perhaps the planner could at some point provide metadata about the params in a
query, e.g. whether they ought to be used in a expression-local (eventually
perhaps also node-local?) way, or query-wide way. Then we could emit a
dedicated expression step for each of the cases, which we can't easily right
now.

Greetings,

Andres Freund