Re: Optimization for updating foreign tables in Postgres FDW

On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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