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