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

Well, it *is* a special case, because it's (ab)using a mechanism
originally meant only for initplans to do something else. Maybe
we should throw out the whole implementation and start over, but
that line of thinking isn't going to lead to something back-patchable.
I'm not very sure what we would do fundamentally differently anyway.

In any case, I don't see what's the problem with Param values being
transmitted locally to an expression. As I hand-waved earlier,
things like CaseTestValue might profitably be treated that way.

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

Yeah, there's room for some optimization there, but I was trying
to minimize the amount of change to the data structures. One idea,
if we don't mind adding another pointer field to ExprState, is to
make a list of just the SubPlans that have private output arrays.
Then, in the vast majority of expressions, that list will be empty
and we needn't do anything much in ExecReadyExpr. I also contemplated
building some intermediate data structure to ease matching of Params
and SubPlans --- however, it's not real clear that that wouldn't cost
more than it saves. We aren't likely to have very many of these
SubPlans in any one query. (Even the specialized-list idea could be
a net loss once you consider the palloc overhead of making a list.
Maybe we should chain the interesting EEOP_SUBPLAN steps together,
similarly to what I did with EEOP_PARAM_EXEC? There's room for a
link field.)

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

Maybe, but then you're adding runtime cost to EEOP_PARAM_EXEC execution
(to check the bitmap) to save compile cost. Doesn't sound promising.

You do have one good point here, which is that we don't really need N
private_exec_vals if there are multiple MULTIEXPR subplans --- they
could share one. But I'm not sure how much contortionism would be
involved in exploiting that observation.

> Afaict we
> don't have cases where single paramid could be used multiple times within a
> single expression?

Right, the paramids should be unique within the expression.

regards, tom lane