On Mon, Sep 4, 2017 at 10:52 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 4 September 2017 at 07:43, Amit Kapila <amit.kapila16@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