Re: Push down more UPDATEs/DELETEs in postgres_fdw

Hi Rushabh,

On 2016/11/22 19:05, Rushabh Lathia wrote:

I started reviewing the patch and here are few initial review points and
questions for you.

Thanks for the review!

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.

No, I don't.  I think the *context should be the last argument as in other functions in deparse.c.  How about inserting that before the out param **retrieved_attrs, like this?

static void
deparseExplicitTargetList(List *tlist,
                          bool is_returning,
                          List **retrieved_attrs,
                          deparse_expr_cxt *context);

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

Will fix.

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?

I don't think it's possible to do that because deparseReturningList uses deparseTargetList internally, which only allows us to emit a target list for a baserel.  For example, deparseReturningList can't handle a returning list that contains join outputs like this:

postgres=# explain verbose delete from ft1 using ft2 where ft1.a = ft2.a returning ft1.*, ft2.*;
                                                       QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
 Delete on public.ft1  (cost=100.00..102.06 rows=1 width=46)
   Output: ft1.a, ft1.b, ft2.a, ft2.b
   ->  Foreign Delete  (cost=100.00..102.06 rows=1 width=46)
         Remote SQL: DELETE FROM public.t1 r1 USING public.t2 r2 WHERE ((r1.a = r2.a)) RETURNING r1.a, r1.b, r2.a, r2.b
(4 rows)

I'd like to change the comment for deparseExplicitTargetList.

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?

You are right.  Will do.

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?

That would be true.  But the fdw_scan_tlist passed from the core would contain junk columns inserted by the rewriter and planner work, such as CTID for the target table and whole-row Vars for other tables specified in the FROM clause of an UPDATE or the USING clause of a DELETE.  So, I created the patch so that the pushed-down UPDATE/DELETE retrieves only the data needed for the RETURNING calculation from the remote server, not the whole fdw_scan_tlist data.

If yes, then why can't we directly replace the fdw_scan_tlist with the
returning
list, rather then make_explicit_returning_list()?

I think we could do that if we modified the core (maybe the executor part).  I'm not sure that's a good idea, though.

Also I think rewriting the fdw_scan_tlist should happen into
postgres_fdw.c -
rather then deparse.c.

Will try.

6) Test coverage for the returning list is missing.

Will add.

Best regards,
Etsuro Fujita