Re: BUG #17809: MERGE ... UPDATE fails with BEFORE ROW UPDATE trigger when target row updated concurrently

On Tue, 7 Mar 2023 at 11:50, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> > A quick hack that fixes the problem is to pass the current
> > MergeActionState to ExecGetUpdateNewTuple(), which also then requires
> > that it be passed to ExecBRUpdateTriggers(). Changing the signature of
> > 2 public functions in that way in back branches doesn't seem very nice
> > though.
> >
> > OTOH, it's difficult to see how it can be fixed any other way.
> >
> > I don't really like this change to ExecGetUpdateNewTuple(), but if we
> > are going to change it like this, then I think we should go all the
> > way and get rid of the GetUpdateNewTuple callback, and the separate
> > internalGetUpdateNewTuple() and mergeGetUpdateNewTuple() functions,
> > and just do it all in ExecGetUpdateNewTuple(), so that all the code is
> > in one place, making it easier to follow.
>
> I'm pretty sure that I split things this way (using a callback) because
> the MERGE code was at arms-length, being in a different file, and there
> was no reasonable way, without duplicating a lot of other code, to
> implement the different behavior other than using a callback.
>
> I then moved the MERGE code from a separate file into nodeModifyTable.c
> and could have put it back in one piece, but didn't remember that it was
> there. It even looks like doing that may even make the code simpler, so
> perhaps we should just do that -- nodeModifyTable.c if fully aware of
> MERGE now, so it's not a problem.
>

So I wrote a patch doing that, and added some isolation tests
confirming that it fixes the reported issue.

Unfortunately, I think this exposes a different issue -- if the target
table has a BEFORE ROW trigger, and the target tuple is concurrently
updated, the trigger code will lock the updated tuple, and the merge
code won't see it as a concurrent update, and so it won't re-check the
WHEN condition. That leads to some quite surprising results, as shown
in the new isolation test cases.

This makes me wonder if perhaps the merge code ought to lock the
target tuple before checking WHEN conditions, in the same way that the
trigger code does before checking trigger WHEN clauses. If it did
that, the trigger code would never have to re-compute the new tuple,
so this change to ExecBRUpdateTriggers() wouldn't be necessary. It
would also probably resolve the issue with cross-partition updates and
concurrently deleted tuples over on [1] and I think the requirement
for cpUpdateRetrySlot would go away. That seems like a pretty invasive
set of changes though.

[1] https://www.postgresql.org/message-id/20230215194224.egfyt3j5ewy4c6m3%40alvherre.pgsql

Regards,
Dean