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

Hi,

On 2023-02-21 17:34:30 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2023-02-21 15:55:15 -0500, Tom Lane wrote:
> >> 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.
>
> I'd really like to *not* need the surrounding plan node to know about
> this. The tlist push-down behavior shown upthread would result in
> that requirement propagating to just about every plan node type,
> certainly every one that allows projection.

Hm, fair point. I had thought about this in a too isolated way, about
expressions evaluated as part of nodeModifyTable.c, rather than stuff
downstream for it.

I was pondering changing EState->es_param_exec_vals while descending into
partition-specific code, just in nodeModifyTable.c, but that doesn't work
as-is, because all the ExprContexts have ->ecxt_param_exec_vals set to the
EState one at node initialization time.

I don't know why we have a copy of es_param_exec_vals in the ExprContext,
tbh. It probably is a tiny bit faster, due to avoiding one level of
indirection, but I have a hard time believing it matters compared to other
costs.

> If we're certain that we'll only need this for MULTIEXPR_SUBLINK and
> thus only in tlists, we could conceivably put the support into
> ExecProject and friends rather than directly in the ExprState
> infrastructure. But that feels like a rather strange compromise,
> and it'd foreclose using the concept for other short-lifespan
> Param-like nodes.

Perhaps we should deal with this by generating a distinct type of expression
step, that looks up information about the param in a different place? Nothing
forces us to have the expression step look into

prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);

If we e.g. emitted a distinct step type for MULTIEXPR_SUBLINK, we could have
it look up params in llast(exprstate->parent->state->es_multilink) that we
push/pop on in nodeModifyTable.c. I think that should work, but it ain't
pretty.

A related idea is be to perform more of the necessary lookups during
expression compilation. If we figured out the execPlan node during expression
compilation, we'd not run into danger of looking up the wrong plan during
expression evaluation.

We already do look into estate during expression compilation for information
about the es_param_list_info, so this wouldn't be breaking new ground.

> Another idea I'm toying with is that the expression compiler could
> allocate some space when it sees a MULTIEXPR_SUBLINK, and then
> connect up the multiexec Params to that.

Where are you thinking of getting the information for connecting the params
from? I don't think we currently have a good way to figure that out during
evaluation time, right?

Greetings,

Andres Freund