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).
