From: | Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com> |
---|---|
To: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
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: | 2016-11-23 11:28:19 |
Message-ID: | CAGPqQf0M9jLR8OiJ55G7nqUU1L44vpp-cR+LU9qfp8O+A=3=xQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Nov 22, 2016 at 6:24 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:
> Hi Rushabh,
>
> On 2016/11/22 19:05, Rushabh Lathia wrote:
>
>> I started reviewing the patch and here are few initial review points and
>> questions for you.
>>
>
> Thanks for the review!
>
> 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.
> 2)
>> -static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
>> - RelOptInfo *joinrel, bool use_alias, List
>> **params_list);
>> +static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
>> RelOptInfo *foreignrel,
>> + bool use_alias, Index target_rel, List
>> **params_list);
>>
>
> Going beyond 80 character length
>>
>
> Will fix.
>
> 3)
>> static void
>> -deparseExplicitTargetList(List *tlist, List **retrieved_attrs,
>> +deparseExplicitTargetList(bool is_returning,
>> + List *tlist,
>> + List **retrieved_attrs,
>> deparse_expr_cxt *context)
>>
>> This looks bit wired to me - as comment for the
>> deparseExplicitTargetList()
>> function name and comments says different then what its trying to do when
>> is_returning flag is passed. Either function comment need to be change or
>> may be separate function fo deparse returning list for join relation?
>>
>> Is it possible to teach deparseReturningList() to deparse the returning
>> list for the joinrel?
>>
>
> I don't think it's possible to do that because deparseReturningList uses
> deparseTargetList internally, which only allows us to emit a target list
> for a baserel. For example, deparseReturningList can't handle a returning
> list that contains join outputs like this:
>
> postgres=# explain verbose delete from ft1 using ft2 where ft1.a = ft2.a
> returning ft1.*, ft2.*;
> QUERY PLAN
> ------------------------------------------------------------
> ------------------------------------------------------------
> Delete on public.ft1 (cost=100.00..102.06 rows=1 width=46)
> Output: ft1.a, ft1.b, ft2.a, ft2.b
> -> Foreign Delete (cost=100.00..102.06 rows=1 width=46)
> Remote SQL: DELETE FROM public.t1 r1 USING public.t2 r2 WHERE
> ((r1.a = r2.a)) RETURNING r1.a, r1.b, r2.a, r2.b
> (4 rows)
>
> I'd like to change the comment for deparseExplicitTargetList.
>
> 4)
>>
>> + if (foreignrel->reloptkind == RELOPT_JOINREL)
>> + {
>> + List *rlist = NIL;
>> +
>> + /* Create a RETURNING list */
>> + rlist = make_explicit_returning_list(rtindex, rel,
>> + returningList,
>> + fdw_scan_tlist);
>> +
>> + deparseExplicitReturningList(rlist, retrieved_attrs, &context);
>> + }
>> + else
>> + deparseReturningList(buf, root, rtindex, rel, false,
>> + returningList, retrieved_attrs);
>>
>> The code will be more centralized if we add the logic of extracting
>> returninglist
>> for JOINREL inside deparseReturningList() only, isn't it?
>>
>
> You are right. 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?
>
>
> Also I think rewriting the fdw_scan_tlist should happen into
>> postgres_fdw.c -
>> rather then deparse.c.
>>
>
> Will try.
That would be good.
>
>
> 6) Test coverage for the returning list is missing.
>>
>
> Will add.
>
>
Thanks.
> Best regards,
> Etsuro Fujita
>
>
>
--
Rushabh Lathia
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2016-11-23 12:07:39 | Re: Dynamic shared memory areas |
Previous Message | Michael Paquier | 2016-11-23 11:24:13 | Re: [RFC] Should we fix postmaster to avoid slow shutdown? |