Re: Optimization for updating foreign tables in Postgres FDW

On 2016/01/12 20:31, Rushabh Lathia wrote:
> On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
>     On 2016/01/06 18:58, Rushabh Lathia wrote:
>         .) 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 ?

Sorry, maybe my explanation was not enough.  Consider:

postgres=# create foreign table ft1 (a int, b int) server myserver 
options (table_name 't1');
postgres=# insert into ft1 values (0, 0);
postgres=# prepare mt(int, int) as update ft1 set a = $1 where b = $2;
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);
postgres=# explain verbose execute mt(1, 0);

After the 5 executions of mt we have

postgres=# explain verbose execute mt(1, 0);                                     QUERY PLAN
------------------------------------------------------------------------------------ Update on public.ft1
(cost=100.00..140.35rows=12 width=10)   ->  Foreign Update on public.ft1  (cost=100.00..140.35 rows=12 width=10)
Remote SQL: UPDATE public.t1 SET a = $1::integer WHERE ((b = 
 
$2::integer))
(3 rows)

If we do that initialization in appendWhereClause, we would get a wrong 
params_list list and a wrong remote pushed-down query for the last mt() 
in deparsePushedDownUpdateSql.

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

Noticed that.  Will do.

BTW, I keep a ForeignScan node pushing down an update to the remote 
server, in the updated patches.  I have to admit that that seems like 
rather a misnomer.  So, it might be worth adding a new ForeignUpdate 
node, but my concern about that is that if doing so, we would have a lot 
of duplicate code in ForeignUpdate and ForeignScan.  What do you think 
about that?

Best regards,
Etsuro Fujita