Re: Push down more UPDATEs/DELETEs in postgres_fdw

On 2016/11/30 17:29, Etsuro Fujita wrote:
> On 2016/11/23 20:28, Rushabh Lathia wrote:

I wrote:
>> 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.

Done.

You wrote:
>> 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?

I wrote:
>> 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().

We could probably avoid that error by replacing the targetlist of the
subplan with fdw_scan_tlist, but that wouldn't be enough ...

You wrote:
>> If yes, then why can't we directly replace the fdw_scan_tlist
>> with the
>> returning
>> list, rather then make_explicit_returning_list()?

I wrote:
>> 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.

The reason why I think so is that the ModifyTable node on top of the
ForeignScan node requires that the targetlist of the ForeignScan has (1)
junk whole-row Vars for secondary relations in UPDATE/DELETE and (2) all
attributes of the target relation to produce the new tuple for UPDATE.
(So, it wouldn't be enough to just replace the ForeignScan's targetlist
with fdw_scan_tlist!) For #1, see this (and the following code) in
ExecInitModifyTable:

/*
* If we have any secondary relations in an UPDATE or DELETE, they
need to
* be treated like non-locked relations in SELECT FOR UPDATE, ie, the
* EvalPlanQual mechanism needs to be told about them. Locate the
* relevant ExecRowMarks.
*/

And for #2, see this (and the following code, especially where calling
ExecCheckPlanOutput) in the same function:

* This section of code is also a convenient place to verify that the
* output of an INSERT or UPDATE matches the target table(s).

What you proposed would be a good idea because the FDW could calculate
the user-query RETURNING list more efficiently in some cases, but I'd
like to leave that for future work.

Attached is the new version of the patch. I also addressed other
comments from you: moved rewriting the fdw_scan_tlist to postgres_fdw.c,
added/revised comments, and added regression tests for the case where a
pushed down UPDATE/DELETE on a join has RETURNING.

My apologies for having been late to work on this.

Best regards,
Etsuro Fujita