On Tue, Nov 22, 2016 at 6:24 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.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
