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

------------------------------------------------------------------------------------------------------------------------
Deleteon 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.06rows=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