Re: Push down more UPDATEs/DELETEs in postgres_fdw

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
Cc: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Push down more UPDATEs/DELETEs in postgres_fdw
Date: 2017-01-25 10:20:22
Message-ID: c78e683e-c0fe-c345-b95b-0742b40a7987@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
postgres-fdw-more-update-pushdown-v2.patch text/x-patch 61.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fabien COELHO 2017-01-25 10:28:00 Re: pgbench more operators & functions
Previous Message Ishii Ayumi 2017-01-25 10:18:26 Re: Radix tree for character conversion