Re: UPDATE of partition key

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