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

Hi,

On 2023-02-21 12:27:54 -0500, Tom Lane wrote:
> If that were the only problem then I'd seriously think about fixing it
> by disallowing this sort of push-down of the UPDATE targetlist when
> MULTIEXPR_SUBLINKs are present.

For a moment I was wondering whether we could do the opposite. Force the
subplan references to be below the Append node. But leaving aside that that
doesn't look trivial, it wouldn't do anything for the insert [oc] case.

> However, the reason we're seeing a comparable problem in ON CONFLICT is that
> ExecInitPartitionInfo *also* makes clones of an UPDATE targetlist, and it is
> far too naive to fix this problem.
>
> Not sure about a good fix. We could imagine porting v13's
> SS_make_multiexprs_unique logic forward into the newer branches,
> but that depends on a lot of planner infrastructure that won't
> be available when ExecInitPartitionInfo runs.

And we really can't change the plan shape much at that point, given that we're
deferring ExecInitPartitionInfo to happen as late as possible.

> I think in the
> back branches we might be forced into disallowing MULTIEXPR_SUBLINKs
> in ON CONFLICT targetlists, because supporting them properly is
> looking like a mess.

Are you thinking of doing that in general, or only for partitioned tables? I'm
worried that this would break a lot of working queries.

Perhaps we could restrict cases of erroring out to when there's a mismatch in
column order between the partitioned table and the partition? IIUC we'll,
somewhat accidentally, do the right thing if the column order matches?

I continue to maintain that it was a seriously bad idea to support this level
of difference between partitioned table and partitions. It adds ugliness and
inefficiencies all over.

> Another angle is that having ExecInitPartitionInfo doing this sort
> of work is a big misallocation of responsibility. If these tlists
> were getting built in the planner it'd be far easier to fix.

Agreed, that doesn't seem quite right. Generally it feels a bunch of
responsibilities that should be more on the planner level have been pushed
down into execPartition.c - but it's not obvious how to fix that in all cases
without increasing the overhead when using runtime partition pruning.

Greetings,

Andres Freund