On 2016/01/06 18:58, Rushabh Lathia wrote:
I started looking at updated patch and its definitely iked the new

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to