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.paquier@gmail.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