On Mon, Sep 4, 2017 at 10:52 AM, Amit Khandekar <amitdkhan...@gmail.com> wrote: > On 4 September 2017 at 07:43, Amit Kapila <amit.kapil...@gmail.com> wrote: > Oops sorry. Now attached.
I have done some basic testing and initial review of the patch. I have some comments/doubts. I will continue the review. + if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture) + ExecARUpdateTriggers(estate, resultRelInfo, InvalidOid, For passing invalid ItemPointer we are using InvalidOid, this seems bit odd to me are we using simmilar convention some other place? I think it would be better to just pass 0? ------ - if ((event == TRIGGER_EVENT_DELETE && delete_old_table) || - (event == TRIGGER_EVENT_UPDATE && update_old_table)) + if (oldtup != NULL && + ((event == TRIGGER_EVENT_DELETE && delete_old_table) || + (event == TRIGGER_EVENT_UPDATE && update_old_table))) { Tuplestorestate *old_tuplestore; - Assert(oldtup != NULL); Only if TRIGGER_EVENT_UPDATE it is possible that oldtup can be NULL, so we have added an extra check for oldtup and removed the Assert, but if TRIGGER_EVENT_DELETE we never expect it to be NULL. Is it better to put Assert outside the condition check (Assert(oldtup != NULL || event == TRIGGER_EVENT_UPDATE)) ? same for the newtup. I think we should also explain in comments about why oldtup or newtup can be NULL in case of if TRIGGER_EVENT_UPDATE ------- + triggers affect the row being moved. As far as <literal>AFTER ROW</> + triggers are concerned, <literal>AFTER</> <command>DELETE</command> and + <literal>AFTER</> <command>INSERT</command> triggers are applied; but + <literal>AFTER</> <command>UPDATE</command> triggers are not applied + because the <command>UPDATE</command> has been converted to a + <command>DELETE</command> and <command>INSERT</command>. Above comments says that ARUpdate trigger is not fired but below code call ARUpdateTrigger + if (mtstate->operation == CMD_UPDATE && mtstate->mt_transition_capture) + ExecARUpdateTriggers(estate, resultRelInfo, InvalidOid, + NULL, + tuple, + NULL, + mtstate->mt_transition_capture); -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers