I started reviewing the patch and here are few initial review points and questions for you.
1) -static void deparseExplicitTargetList(List *tlist, List **retrieved_attrs, +static void deparseExplicitTargetList(bool is_returning, + List *tlist, + List **retrieved_attrs, deparse_expr_cxt *context); Any particular reason of inserting new argument as 1st argunment? In general we add new flags at the end or before the out param for the existing function. 2) -static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root, - RelOptInfo *joinrel, bool use_alias, List **params_list); +static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel, + bool use_alias, Index target_rel, List **params_list); Going beyond 80 character length 3) static void -deparseExplicitTargetList(List *tlist, List **retrieved_attrs, +deparseExplicitTargetList(bool is_returning, + List *tlist, + List **retrieved_attrs, deparse_expr_cxt *context) This looks bit wired to me - as comment for the deparseExplicitTargetList() function name and comments says different then what its trying to do when is_returning flag is passed. Either function comment need to be change or may be separate function fo deparse returning list for join relation? Is it possible to teach deparseReturningList() to deparse the returning list for the joinrel? 4) + if (foreignrel->reloptkind == RELOPT_JOINREL) + { + List *rlist = NIL; + + /* Create a RETURNING list */ + rlist = make_explicit_returning_list(rtindex, rel, + returningList, + fdw_scan_tlist); + + deparseExplicitReturningList(rlist, retrieved_attrs, &context); + } + else + deparseReturningList(buf, root, rtindex, rel, false, + returningList, retrieved_attrs); The code will be more centralized if we add the logic of extracting returninglist for JOINREL inside deparseReturningList() only, isn't it? 5) make_explicit_returning_list() pull the var list from the returningList and build the targetentry for the returning list and at the end rewrites the fdw_scan_tlist. AFAIK, in case of DML - which is going to pushdown to the remote server ideally fdw_scan_tlist should be same as returning list, as final output for the query is query will be RETUNING list only. isn't that true? If yes, then why can't we directly replace the fdw_scan_tlist with the returning list, rather then make_explicit_returning_list()? Also I think rewriting the fdw_scan_tlist should happen into postgres_fdw.c - rather then deparse.c. 6) Test coverage for the returning list is missing. On Fri, Nov 18, 2016 at 8:56 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2016/11/16 16:38, Etsuro Fujita wrote: > >> On 2016/11/16 13:10, Ashutosh Bapat wrote: >> >>> I don't see any reason why DML/UPDATE pushdown should depend upon >>> subquery deparsing or least PHV patch. Combined together they can help >>> in more cases, but without those patches, we will be able to push-down >>> more stuff. Probably, we should just restrict push-down only for the >>> cases when above patches are not needed. That makes reviews easy. Once >>> those patches get committed, we may add more functionality depending >>> upon the status of this patch. Does that make sense? >>> >> > OK, I'll extract from the patch the minimal part that wouldn't depend on >> the two patches. >> > > Here is a patch for that. Todo items are: (1) add more comments and (2) > add more regression tests. I'll do that in the next version. > > Best regards, > Etsuro Fujita > -- Rushabh Lathia