Re: Optimization for updating foreign tables in Postgres FDW

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