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.
> .) 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.
I think this depends on the capabilities of the FDW.
> .) Documentation for the new API is missing (fdw-callbacks).
Will add the docs.
Best regards,
Etsuro Fujita
