Re: [PATCH] Make ON CONFLICT DO NOTHING and ON CONFLICT DO UPDATE consistent

Hi,

On 2023-01-25 22:00:50 +0300, Aleksander Alekseev wrote:
> Perhaps that's not a bug especially considering the fact that the
> documentation describes this behavior, but in any case the fact that:
>
> ```
> INSERT INTO t VALUES (1,1) ON CONFLICT (a) DO UPDATE SET b = 0;
> INSERT INTO t VALUES (1,2) ON CONFLICT (a) DO UPDATE SET b = 0;
> ```
>
> and:
>
> ```
> INSERT INTO t VALUES (1,1), (1,2) ON CONFLICT (a) DO NOTHING;
> ``
>
> .. both work, and:
>
> ```
> INSERT INTO t VALUES (1,1), (1,2) ON CONFLICT (a) DO UPDATE SET b = 0;
> ```
>
> ... doesn't is rather confusing. There is no reason why the latest
> query shouldn't work except for a slight complication of the code.
> Which seems to be a reasonable tradeoff, for me at least.

I don't agree that this is just about a "slight complication" of the code. I
think semantically the proposed new behaviour is pretty bogus.

It *certainly* can't be right to just continue with the update in heap_update,
as you've done. You'd have to skip the update, not execute it. What am I
missing here?

I think this'd completely break triggers, for example, because they won't be
able to get the prior row version, since it won't actually be a row ever
visible (due to cmin=cmax).

I suspect it might break unique constraints as well, because we'd end up with
an invisible row in part of the ctid chain.

> > But what's the justification for erroring out in the DO NOTHING case?
> >
> > [...]
> >
> > It seems somewhat likely that a behavioural change will cause trouble for some
> > of the uses of DO NOTHING out there.
>
> Just to make sure we are on the same page. The patch doesn't break the
> current DO NOTHING behavior but rather makes DO UPDATE work the same
> way DO NOTHING does.

I see that now - I somehow thought you were recommending to error out in both
cases, rather than the other way round.

Greetings,

Andres Freund