Re: [HACKERS] 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers