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

[Ashutosh B. <ash...@en...>]

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?


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?
>
> Please use RelationGetLocInfo to get location info given Relation in
> 1336         if
> (!IsLocatorReplicated(GetLocatorType(RelationGetRelid(target_relation))))
>
> 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