Re: Optimization for updating foreign tables in Postgres FDW - Mailing list pgsql-hackers

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.35 rows=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?