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

On Fri, Dec 4, 2015 at 6:10 AM, Peter Geoghegan <pg@heroku.com> wrote:
> 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.

I am adding that to the list of open items for 9.5.
--
Michael