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