Re: Optimization for updating foreign tables in Postgres FDW

On 2016/01/14 21:36, Rushabh Lathia wrote:
> On Thu, Jan 14, 2016 at 2:00 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:

> On 2016/01/12 20:31, Rushabh Lathia wrote:

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

Best regards,
Etsuro Fujita