Re: Push down more UPDATEs/DELETEs in postgres_fdw

On 2016/11/23 20:28, Rushabh Lathia wrote:
> On Tue, Nov 22, 2016 at 6:24 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

>         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);

> Yes, adding it before retrieved_attrs would be good.

OK, 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.

> Yes, I noticed that fdw_scan_tlist have CTID for the target table and
> whole-raw vars for
> other tables specified in the FROM clause of the DML. But I was thinking
> isn't it possible
> to create new fdw_scan_tlist once we found that DML is direct pushable
> to foreign server?
> I tried quickly doing that - but later its was throwing "variable not
> found in subplan target list"
> from set_foreignscan_references().

>         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.

> Yes modifying core is not good idea. But just want to understand why you
> think that this need need to modify core?

Sorry, I don't remember that.  Will check.

I'd like to move this to the next CF.

Thank you for the comments!

Best regards,
Etsuro Fujita