Re: Optimization for updating foreign tables in Postgres FDW

On Thu, Jan 14, 2016 at 2:00 PM, Etsuro Fujita <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>> 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)

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

Regards,

--
Rushabh Lathia
www.EnterpriseDB.come