Re: Push down more UPDATEs/DELETEs in postgres_fdw - Mailing list pgsql-hackers

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.