Re: Incorrect UPDATE trigger invocation in the UPDATE clause of an UPSERT statement.

On Thu, Dec 3, 2015 at 4:34 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> triggers.out is telling that this may be intended to work this way:
> WARNING: after update (old): (5,"updated green trig modified")
> WARNING: after update (new): (5,"updated green trig modified")
> Though I agree that it is not instinctive...
>
> Btw, the patch provided fails on an assertion with regression tests:
> 2426 /* Determine lock mode to use */
> 2427 lockmode = ExecUpdateLockMode(estate, relinfo);
> 2428
> -> 2429 Assert(HeapTupleIsValid(fdw_trigtuple) ^
> ItemPointerIsValid(tupleid));
> 2430 if (fdw_trigtuple == NULL)
>
> Peter, Andres, thoughts?

I agree with Stanislav that the behavior is wrong. The fix is more
complicated, though. As it says at the top of ExecUpdate():

* When updating a table, tupleid identifies the tuple to
* update and oldtuple is NULL. When updating a view, oldtuple

Since the ON CONFLICT DO UPDATE variant is always updating a table, it
is always supposed to pass oldtuple = NULL. The problem is that unlike
a regular update, it cannot reconstruct the original/target tuple for
an AFTER UPDATE trigger *correctly* -- the GetTupleForTrigger() logic
depends on executor state, which is not consistent with a regular
update.

I'll need to think about a fix.

--
Peter Geoghegan