|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|
|Views:||Raw Message | Whole Thread | Download mbox|
On 2016/11/30 17:29, Etsuro Fujita wrote:
> On 2016/11/23 20:28, Rushabh Lathia 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.
>> 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
>> AFAIK, in case of DML - which is going to pushdown to the remote
>> 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
>> 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
>> 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 ...
>> If yes, then why can't we directly replace the fdw_scan_tlist
>> with the
>> 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.
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
* If we have any secondary relations in an UPDATE or DELETE, they
* 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.
|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|