Re: Push down more UPDATEs/DELETEs in postgres_fdw

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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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