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

On 2016/01/14 21:36, Rushabh Lathia wrote:

On Thu, Jan 14, 2016 at 2:00 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

    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>
        <mailto: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.

Strange, I am seeing same behaviour with or without that initialization in
appendWhereClause. After the 5 executions of mt I with or without I am
getting following output:

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

Really?  With that initialization in appendWhereClause, I got the following wrong result (note that both parameter numbers are $1):

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 = $1::integer))
(3 rows)

    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?

Yes, I noticed that in the patch and I was about to point that out in my
final review. As first review I was mainly focused on the functionality
testing
and other overview things. Another reason I haven't posted that in my
first review round is, I was not quite sure whether we need the
separate new node ForeignUpdate, ForeignDelete  and want to duplicate
code? Was also not quite sure about the fact that what we will achieve
by doing that.

So I thought, I will have this open question in my final review comment,
and will take committer's opinion on this. Since you already raised this
question lets take others opinion on this.

OK, let's do that.