Re: Push down more UPDATEs/DELETEs in postgres_fdw

<div dir="ltr">I started reviewing the patch and here are few initial review points and questions for you.<br /><br
/>1)<br/>-static void deparseExplicitTargetList(List *tlist, List **retrieved_attrs,<br />+static void
deparseExplicitTargetList(boolis_returning,<br />+                          List *tlist,<br />+                       
 List **retrieved_attrs,<br />                           deparse_expr_cxt *context);<br /><br />Any particular reason
ofinserting new argument as 1st argunment? In general<br />we add new flags at the end or before the out param for the
existingfunction.<br /><br />2) <br />-static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,<br />-   
               RelOptInfo *joinrel, bool use_alias, List **params_list);<br />+static void
deparseFromExprForRel(StringInfobuf, PlannerInfo *root, RelOptInfo *foreignrel,<br />+                      bool
use_alias,Index target_rel, List **params_list);<br /><br /><br />Going beyond 80 character length<br /><br />3)<br
/> staticvoid<br />-deparseExplicitTargetList(List *tlist, List **retrieved_attrs,<br />+deparseExplicitTargetList(bool
is_returning,<br/>+                          List *tlist,<br />+                          List **retrieved_attrs,<br
/>                          deparse_expr_cxt *context)<br /><br />This looks bit wired to me - as comment for the
deparseExplicitTargetList()<br/>function name and comments says different then what its trying to do when<br
/>is_returningflag is passed. Either function comment need to be change or<br />may be separate function fo deparse
returninglist for join relation?<br /><br />Is it possible to teach deparseReturningList() to deparse the returning<br
/>listfor the joinrel?<br /><br />4)<br /><br />+    if (foreignrel->reloptkind == RELOPT_JOINREL)<br />+    {<br
/>+       List       *rlist = NIL;<br />+<br />+        /* Create a RETURNING list */<br />+        rlist =
make_explicit_returning_list(rtindex,rel,<br />+                                             returningList,<br />+   
                                        fdw_scan_tlist);<br />+<br />+        deparseExplicitReturningList(rlist,
retrieved_attrs,&context);<br />+    }<br />+    else<br />+        deparseReturningList(buf, root, rtindex, rel,
false,<br/>+                             returningList, retrieved_attrs);<br /><br />The code will be more centralized
ifwe add the logic of extracting returninglist<br />for JOINREL inside deparseReturningList() only, isn't it?<br /><br
/>5)make_explicit_returning_list() pull the var list from the returningList and<br />build the targetentry for the
returninglist and at the end rewrites the<br />fdw_scan_tlist. <br /><br />AFAIK, in case of DML - which is going to
pushdownto the remote server <br />ideally fdw_scan_tlist should be same as returning list, as final output<br />for
thequery is query will be RETUNING list only. isn't that true?<br /><br />If yes, then why can't we directly replace
thefdw_scan_tlist with the returning<br />list, rather then make_explicit_returning_list()?<br /><br />Also I think
rewritingthe fdw_scan_tlist should happen into postgres_fdw.c -<br />rather then deparse.c.<br /><br />6) Test coverage
forthe returning list is missing.<br /><br /><br /></div><div class="gmail_extra"><br /><div class="gmail_quote">On
Fri,Nov 18, 2016 at 8:56 AM, Etsuro Fujita <span dir="ltr"><<a href="mailto:fujita.etsuro@lab.ntt.co.jp"
target="_blank">fujita.etsuro@lab.ntt.co.jp</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0
0.8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 2016/11/16 16:38, Etsuro Fujita wrote:<br
/></span><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span
class="">On 2016/11/16 13:10, Ashutosh Bapat wrote:<br /></span><span class=""><blockquote class="gmail_quote"
style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I don't see any reason why DML/UPDATE pushdown
shoulddepend upon<br /> subquery deparsing or least PHV patch. Combined together they can help<br /> in more cases, but
withoutthose patches, we will be able to push-down<br /> more stuff. Probably, we should just restrict push-down only
forthe<br /> cases when above patches are not needed. That makes reviews easy. Once<br /> those patches get committed,
wemay add more functionality depending<br /> upon the status of this patch. Does that make sense?<br
/></blockquote></span></blockquote><spanclass=""><br /><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px#ccc solid;padding-left:1ex"> OK, I'll extract from the patch the minimal part that wouldn't depend
on<br/> the two patches.<br /></blockquote><br /></span> Here is a patch for that.  Todo items are: (1) add more
commentsand (2) add more regression tests.  I'll do that in the next version.<br /><br /> Best regards,<br /> Etsuro
Fujita<br/></blockquote></div><br /><br clear="all" /><br />-- <br /><div class="gmail_signature"
data-smartmail="gmail_signature">RushabhLathia</div></div>