Re: [Postgres-xc-developers] Review comments for commit: 31cf556fdb44d0872998f37ac222e0ac6f1faeb6

[Amit K. <ami...@en...>]

On 10 April 2013 16:36, Ashutosh Bapat <ash...@en...>wrote:

> Hi Amit,
> Sorry, there is one more comment,
> I see that the code to add wholerow attribute has been duplicated, once
> for views and once for before row triggers. Is it possible not to duplicate
> this code?
>
>     Yes, wholerow attribute has been duplicated, but I think it is simpler
to keep
    it like that. I had attempted to merge it in PG code but the code
becomes
    complicated and it is difficult understand exactly which junk
attribtues are
    added and for which node, and which type of table or view.


>
> On Wed, Apr 10, 2013 at 4:33 PM, Ashutosh Bapat <
> ash...@en...> wrote:
>
>> Hi Amit
>> Not your change but, we need to update the prologue of
>> rewriteTargetListUD with XC specific changes.
>>
>> Again not your change, but there's comment in the function
>> 1256     /*
>> 1257      * In Postgres-XC, we need to evaluate quals of the parse tree
>> and determine
>> 1258      * if they are Coordinator quals. If they are, their attribute
>> need to be
>> 1259      * added to target list for evaluation. In case some are found,
>> add them as
>> 1260      * junks in the target list. The junk status will be used by
>> remote UPDATE
>> 1261      * planning to associate correct element to a clause.
>> 1262      * For DELETE, having such columns in target list helps to
>> evaluate Quals
>> 1263      * correctly on Coordinator.
>> 1264      * PGXCTODO: This list could be reduced to keep only in target
>> list the
>> 1265      * vars using Coordinator Quals.
>> 1266      */
>> 1267     if (IS_PGXC_COORDINATOR && parsetree->jointree)
>> 1268         var_list = pull_qual_vars((Node *) parsetree->jointree,
>> parsetree->resultRelation);
>>
>> I think we need to do this only when these attributes are not in the
>> targetlist already. My guess is we don't need this code at all. Can we
>> check this?
>>
>
    The pull_qual_vars() code is not related to trigger related changes. I
have
    anyways removed this code in the patch that I submitted for fetching
only
    required columns for DML source data query.


>
>> Please use RelationGetLocInfo to get location info given Relation in
>> 1336         if
>> (!IsLocatorReplicated(GetLocatorType(RelationGetRelid(target_relation))))
>>
>
    Used RelationGetLocInfo() instead of GetLocatorType(RelationGetRelid...
although
    again this was an existing code, but I anyways corrected it because it
was quite
    close to the trigger changes.

The commit id for the above changes is :
44d07c5c1a405e5b896403741f8f2aea288d0313 (rowtriggers_new)


>> Rest of the changes look fine.
>> --
>> Best Wishes,
>> Ashutosh Bapat
>> EntepriseDB Corporation
>> The Enterprise Postgres Company
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EntepriseDB Corporation
> The Enterprise Postgres Company
>