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.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 = 
 
$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.

Best regards,
Etsuro Fujita