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(at)heroku(dot)com> wrote:
> 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.

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