Re: UPDATE of partition key

On 21 September 2017 at 19:52, amul sul <sulamul@gmail.com> wrote:
> On Wed, Sep 20, 2017 at 9:27 PM, Amit Khandekar <amitdkhan.pg@gmail.com>
> wrote:
>>
>> On 20 September 2017 at 00:06, Robert Haas <robertmhaas@gmail.com> wrote:
>> > On Fri, Sep 15, 2017 at 7:25 AM, Amit Khandekar <amitdkhan.pg@gmail.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