Re: UPDATE of partition key

On 21 September 2017 at 19:52, amul sul <sulamul(at)gmail(dot)com> wrote:
> On Wed, Sep 20, 2017 at 9:27 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>
> wrote:
>>
>> On 20 September 2017 at 00:06, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > On Fri, Sep 15, 2017 at 7:25 AM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>
>> > wrote:
>> >> [ new patch ]
>
>
> 86 - (event == TRIGGER_EVENT_UPDATE &&
> !trigdesc->trig_update_after_row))
> 87 + (event == TRIGGER_EVENT_UPDATE &&
> !trigdesc->trig_update_after_row) ||
> 88 + (event == TRIGGER_EVENT_UPDATE && (oldtup == NULL || newtup
> == NULL)))
> 89 return;
> 90 }
>
>
> Either of oldtup or newtup will be valid at a time & vice versa. Can we
> improve
> this check accordingly?
>
> For e.g.:
> (event == TRIGGER_EVENT_UPDATE && )(HeapTupleIsValid(oldtup) ^
> ItemPointerIsValid(newtup)))))

Ok, I will be doing this as below :
- (event == TRIGGER_EVENT_UPDATE && (oldtup == NULL || newtup == NULL)))
+ (event == TRIGGER_EVENT_UPDATE && ((oldtup == NULL) ^ (newtup == NULL))))

At other places in the function, oldtup and newtup are checked for
NULL, so to be consistent, I haven't used HeapTupleIsValid.

Actually, it won't happen that both oldtup and newtup are NULL ... in
either of delete, insert, or update, but I haven't added an Assert for
this, because that has been true even on HEAD.

Will include the above minor change in the next patch when more changes come in.

>
>
> 247
> 248 + /*
> 249 + * EDB: In case this is part of update tuple routing, put this row
> into the
> 250 + * transition NEW TABLE if we are capturing transition tables. We
> need to
> 251 + * do this separately for DELETE and INSERT because they happen on
> 252 + * different tables.
> 253 + */
> 254 + if (mtstate->operation == CMD_UPDATE &&
> mtstate->mt_transition_capture)
> 255 + ExecARUpdateTriggers(estate, resultRelInfo, NULL,
> 256 + NULL,
> 257 + tuple,
> 258 + NULL,
> 259 + mtstate->mt_transition_capture);
> 260 +
> 261 list_free(recheckIndexes);
>
> 267
> 268 + /*
> 269 + * EDB: In case this is part of update tuple routing, put this row
> into the
> 270 + * transition OLD TABLE if we are capturing transition tables. We
> need to
> 271 + * do this separately for DELETE and INSERT because they happen on
> 272 + * different tables.
> 273 + */
> 274 + if (mtstate->operation == CMD_UPDATE &&
> mtstate->mt_transition_capture)
> 275 + ExecARUpdateTriggers(estate, resultRelInfo, tupleid,
> 276 + oldtuple,
> 277 + NULL,
> 278 + NULL,
> 279 + mtstate->mt_transition_capture);
> 280 +
>
> Initially, I wondered that why can't we have above code right after
> ExecInsert() & ExecIDelete() in ExecUpdate respectively?
>
> We can do that for ExecIDelete() but not easily in the ExecInsert() case,
> because ExecInsert() internally searches the correct partition's
> resultRelInfo
> for an insert and before returning to ExecUpdate resultRelInfo is restored
> to the old one. That's why current logic seems to be reasonable for now.
> Is there anything that we can do?

Yes, resultRelInfo is different when we return from ExecInsert().
Also, I think the trigger and transition capture be done immediately
after the rows are inserted. This is true for existing code also.
Furthermore, there is a dependency of ExecARUpdateTriggers() on
ExecARInsertTriggers(). transition_capture is passed NULL if we
already captured the tuple in ExecARUpdateTriggers(). It looks simpler
to do all this at a single place.

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company