On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
> On 2016/01/06 18:58, Rushabh Lathia wrote: > >> I started looking at updated patch and its definitely iked the new >> approach. >> > > Thanks for the review! > > With the initial look and test overall things looking great, I am still >> reviewing the code changes but here are few early doubts/questions: >> > > .) What the need of following change ? >> >> @@ -833,9 +833,6 @@ appendWhereClause(StringInfo buf, >> int nestlevel; >> ListCell *lc; >> >> - if (params) >> - *params = NIL; /* initialize result list to empty */ >> - >> /* Set up context struct for recursion */ >> context.root = root; >> context.foreignrel = baserel; >> @@ -971,6 +968,63 @@ deparseUpdateSql(StringInfo buf, PlannerInfo *root, >> } >> > > It is needed for deparsePushedDownUpdateSql to store params in both WHERE > clauses and expressions to assign to the target columns > into one params_list list. > Hmm sorry but I am still not getting the point, can you provide some example to explain this ? .) When Tom Lane and Stephen Frost suggested getting the core code involved, >> I thought that we can do the mandatory checks into core it self and making >> completely out of dml_is_pushdown_safe(). Please correct me >> > > The reason why I put that function in postgres_fdw.c is Check point 4: > > + * 4. We can't push an UPDATE down, if any expressions to assign to the > target > + * columns are unsafe to evaluate on the remote server. > > Here I was talking about checks related to triggers, or to LIMIT. I think earlier thread talked about those mandatory check to the core. So may be we can move those checks into make_modifytable() before calling the PlanDMLPushdown. I think this depends on the capabilities of the FDW. > > .) Documentation for the new API is missing (fdw-callbacks). >> > > Will add the docs. > -- Rushabh Lathia
