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

Hi,

On 2023-02-21 17:17:05 -0800, Andres Freund wrote:
> We currently overwrite prm->execPlan in ExecInitSubPlan(), when creating a
> second reference to the subplan. Which is why we then end up using the wrong
> SubPlanState in ExecSetParamPlan().
>
> The problem of using the wrong SubPlanState doesn't look too hard to solve: We
> could stash the "actual" execPlan in scratch.d.param.something, instead of
> looking it up during ExecEvalParamExec().

It's not quite that easy, because there can be references to subplans that
aren't subquery specific.

I did come up with a, very hacky at this point, prototype that does seems to
fix the issue.

The main problem here IMO is that
a) there can be many different ParamExecData->execPlans for a single param
b) we only want a single value for a PARAM_EXEC to be valid at a time

One solution to that is to move execPlans out of ParamExecData.

In the attached prototype I added a two dimensional list to EState. The first
level is indexed by the paramid, the second identifies partition specific
plans. For non-partition specific subplans it is 0, for partition specific
ones it is a new integer assigned sequentially within ExecInitPartitionInfo().

When generating a reference to a PARAM_EXEC parameter, the current
partition-specific offset is added to the expression step.

One significant complication is that we can't actually know at the point we
encounter the parameter whether the subplan is partition specific or not, or
at least it wasn't obvious to me how to do so. So ExecEvalParamExec() has to
try both.

Obviously the attached patch is in no way meant to be more than a proof of
concept.

I don't think we can move all of the responsibilities here to the planner - we
don't want to generate a plan that has pre-generated expressions, with
different param ids, for every potentially affected partition. So I do think
we need some runtime way of identifying the correct execPlan.

But I think the planner could make the executors job easier, by providing
conveniently accessible information about what a specific paramid is used
for. If we e.g. knew that a specific Param
a) will require execPlan processing
b) references an expression that is expected to be partition specific

we could figure out during expression compilation whether to make the
expression step reference the "query global" plan, or not. Instead of
deferring that to a runtime check in ExecEvalParamExec().

The attached really is just an exploration of the idea, not something anywhere
near a real fix.

It also doesn't yet address the issue with the wrong subplan chosen when the
planner expands partitions, as that doesn't go through
ExecInitPartitionInfo(). But I'm not sure that should be addressed by the same
mechanism - that seems a bit more the planner's responsibility. This means
we'll continue to fail to evaluate
explain (verbose, analyze) UPDATE t SET (a, b) = (SELECT t.a, t.b || '+')
except that now we'll print
WARNING: 01000: overwriting previous subplan exec plan: 0
LOCATION: ExecInitSubPlan, nodeSubplan.c:884
which probably ought to at least be an assertion failure if not a runtime
ERROR, once we hit that, things won't work reliably.

While hacking on this I found it helpful to have ruleutils/explain provide a
bit more detail:
- have references to subplans print the arguments
- deparse onConflictSet
- print subplans if evaluated in the context of a different node

Not sure if any of that is interesting enough to be worth doing outside of
hacking on code like this.

Greetings,

Andres Freund