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

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