Optimization for updating foreign tables in Postgres FDW

Attached is a WIP patch for the following:

/*
* postgresPlanForeignModify
* Plan an insert/update/delete operation on a foreign table
*
* Note: currently, the plan tree generated for UPDATE/DELETE will always
* include a ForeignScan that retrieves ctids (using SELECT FOR UPDATE)
* and then the ModifyTable node will have to execute individual remote
* UPDATE/DELETE commands. If there are no local conditions or joins
* needed, it'd be better to let the scan node do UPDATE/DELETE RETURNING
* and then do nothing at ModifyTable. Room for future optimization ...
*/

In the patch postgresPlanForeignModify has been modified so that if, in
addition to the above condition, the followings are satisfied, then the
ForeignScan and ModifyTable node will work that way.

- There are no local BEFORE/AFTER triggers.
- In UPDATE it's safe to evaluate expressions to assign to the target
columns on the remote server.

Here is a simple performance test.

On remote side:
postgres=# create table t (id serial primary key, inserted timestamp
default clock_timestamp(), data text);
CREATE TABLE
postgres=# insert into t(data) select random() from generate_series(0,
99999);
INSERT 0 100000
postgres=# vacuum t;
VACUUM

On local side:
postgres=# create foreign table ft (id integer, inserted timestamp, data
text) server myserver options (table_name 't');
CREATE FOREIGN TABLE

Unpatched:
postgres=# explain analyze verbose delete from ft where id < 10000;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Delete on public.ft (cost=100.00..162.32 rows=910 width=6) (actual
time=1275.255..1275.255 rows=0 loops=1)
Remote SQL: DELETE FROM public.t WHERE ctid = $1
-> Foreign Scan on public.ft (cost=100.00..162.32 rows=910 width=6)
(actual time=1.180..52.095 rows=9999 loops=1)
Output: ctid
Remote SQL: SELECT ctid FROM public.t WHERE ((id < 10000)) FOR
UPDATE
Planning time: 0.112 ms
Execution time: 1275.733 ms
(7 rows)

Patched (Note that the DELETE command has been pushed down.):
postgres=# explain analyze verbose delete from ft where id < 10000;
QUERY PLAN
-------------------------------------------------------------------------------------------------------------------
Delete on public.ft (cost=100.00..162.32 rows=910 width=6) (actual
time=0.006..0.006 rows=0 loops=1)
-> Foreign Scan on public.ft (cost=100.00..162.32 rows=910 width=6)
(actual time=0.001..0.001 rows=0 loops=1)
Output: ctid
Remote SQL: DELETE FROM public.t WHERE ((id < 10000))
Planning time: 0.101 ms
Execution time: 8.808 ms
(6 rows)

I'll add this to the next CF. Comments are welcome.

Thanks,

Best regards,
Etsuro Fujita

On Tue, Jul 8, 2014 at 3:07 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Attached is a WIP patch for the following:
>
> /*
> * postgresPlanForeignModify
> * Plan an insert/update/delete operation on a foreign table
> *
> * Note: currently, the plan tree generated for UPDATE/DELETE will always
> * include a ForeignScan that retrieves ctids (using SELECT FOR UPDATE)
> * and then the ModifyTable node will have to execute individual remote
> * UPDATE/DELETE commands. If there are no local conditions or joins
> * needed, it'd be better to let the scan node do UPDATE/DELETE RETURNING
> * and then do nothing at ModifyTable. Room for future optimization ...
> */
>
> In the patch postgresPlanForeignModify has been modified so that if, in
> addition to the above condition, the followings are satisfied, then the
> ForeignScan and ModifyTable node will work that way.
>
> - There are no local BEFORE/AFTER triggers.
> - In UPDATE it's safe to evaluate expressions to assign to the target
> columns on the remote server.
>
> Here is a simple performance test.
>
> On remote side:
> postgres=# create table t (id serial primary key, inserted timestamp
> default clock_timestamp(), data text);
> CREATE TABLE
> postgres=# insert into t(data) select random() from generate_series(0,
> 99999);
> INSERT 0 100000
> postgres=# vacuum t;
> VACUUM
>
> On local side:
> postgres=# create foreign table ft (id integer, inserted timestamp, data
> text) server myserver options (table_name 't');
> CREATE FOREIGN TABLE
>
> Unpatched:
> postgres=# explain analyze verbose delete from ft where id < 10000;
> QUERY PLAN
> -----------------------------------------------------------------------------------------------------------------------
> Delete on public.ft (cost=100.00..162.32 rows=910 width=6) (actual
> time=1275.255..1275.255 rows=0 loops=1)
> Remote SQL: DELETE FROM public.t WHERE ctid = $1
> -> Foreign Scan on public.ft (cost=100.00..162.32 rows=910 width=6)
> (actual time=1.180..52.095 rows=9999 loops=1)
> Output: ctid
> Remote SQL: SELECT ctid FROM public.t WHERE ((id < 10000)) FOR
> UPDATE
> Planning time: 0.112 ms
> Execution time: 1275.733 ms
> (7 rows)
>
> Patched (Note that the DELETE command has been pushed down.):
> postgres=# explain analyze verbose delete from ft where id < 10000;
> QUERY PLAN
> -------------------------------------------------------------------------------------------------------------------
> Delete on public.ft (cost=100.00..162.32 rows=910 width=6) (actual
> time=0.006..0.006 rows=0 loops=1)
> -> Foreign Scan on public.ft (cost=100.00..162.32 rows=910 width=6)
> (actual time=0.001..0.001 rows=0 loops=1)
> Output: ctid
> Remote SQL: DELETE FROM public.t WHERE ((id < 10000))
> Planning time: 0.101 ms
> Execution time: 8.808 ms
> (6 rows)
>
> I'll add this to the next CF. Comments are welcome.

I haven't looked at the code, but +1 for the general idea. The
concept seems good to me, and that's a very large performance
improvement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Fujita-san,

My coworker Takaaki EITOKU reviewed the patch, and here are some
comments from him.

2014-07-08 16:07 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
...
> In the patch postgresPlanForeignModify has been modified so that if, in
> addition to the above condition, the followings are satisfied, then the
> ForeignScan and ModifyTable node will work that way.
>
> - There are no local BEFORE/AFTER triggers.

The reason the check ignores INSTEAD OF triggers here is that INSTEAD
OF trigger would prevent executing UPDATE/DELETE statement against a
foreign tables at all, right?

> - In UPDATE it's safe to evaluate expressions to assign to the target
> columns on the remote server.

IIUC, in addition to target expressions, whole WHERE clause should be
safe to be pushed-down. Though that is obviously required, but
mentioning that in documents and source comments would help users and
developers.

>
> Here is a simple performance test.
>
> On remote side:
> postgres=# create table t (id serial primary key, inserted timestamp
> default clock_timestamp(), data text);
> CREATE TABLE
> postgres=# insert into t(data) select random() from generate_series(0,
> 99999);
> INSERT 0 100000
> postgres=# vacuum t;
> VACUUM
>
> On local side:
> postgres=# create foreign table ft (id integer, inserted timestamp, data
> text) server myserver options (table_name 't');
> CREATE FOREIGN TABLE
>
> Unpatched:
> postgres=# explain analyze verbose delete from ft where id < 10000;
> QUERY PLAN
> -----------------------------------------------------------------------------------------------------------------------
> Delete on public.ft (cost=100.00..162.32 rows=910 width=6) (actual
> time=1275.255..1275.255 rows=0 loops=1)
> Remote SQL: DELETE FROM public.t WHERE ctid = $1
> -> Foreign Scan on public.ft (cost=100.00..162.32 rows=910 width=6)
> (actual time=1.180..52.095 rows=9999 loops=1)
> Output: ctid
> Remote SQL: SELECT ctid FROM public.t WHERE ((id < 10000)) FOR
> UPDATE
> Planning time: 0.112 ms
> Execution time: 1275.733 ms
> (7 rows)
>
> Patched (Note that the DELETE command has been pushed down.):
> postgres=# explain analyze verbose delete from ft where id < 10000;
> QUERY PLAN
> -------------------------------------------------------------------------------------------------------------------
> Delete on public.ft (cost=100.00..162.32 rows=910 width=6) (actual
> time=0.006..0.006 rows=0 loops=1)
> -> Foreign Scan on public.ft (cost=100.00..162.32 rows=910 width=6)
> (actual time=0.001..0.001 rows=0 loops=1)
> Output: ctid
> Remote SQL: DELETE FROM public.t WHERE ((id < 10000))
> Planning time: 0.101 ms
> Execution time: 8.808 ms
> (6 rows)

We found that this patch speeds up DELETE case remarkably, as you
describe above, but we saw only less than 2x speed on UPDATE cases.
Do you have any numbers of UPDATE cases?

Some more random thoughts:

* Naming of new behavior
You named this optimization "Direct Update", but I'm not sure that
this is intuitive enough to express this behavior. I would like to
hear opinions of native speakers.

* Macros for # of fdw_private elements
In postgres_fdw.c you defined MaxFdwScanFdwPrivateLength and
MinFdwScanFdwPrivateLength for determining the mode, but number of
fdw_private elements is not a ranged value but an enum value (I mean
that fdw_private holds 3 or 7, not 4~6, so Max and Min seems
inappropriate for prefix. IMO those macros should be named so that
the names represent the behavior, or define a macro to determine the
mode, say IS_SHIPPABLE(foo).

* Comparison of Macros
Comparison against MaxFdwScanFdwPrivateLength and
MinFdwScanFdwPrivateLength should be == instead of >= or <= to detect
unexpected value. Adding assert macro seems good too.

--
Shigeru HANADA

(2014/07/24 18:30), Shigeru Hanada wrote:
> My coworker Takaaki EITOKU reviewed the patch, and here are some
> comments from him.

Thank you for the review, Eitoku-san!

> 2014-07-08 16:07 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> In the patch postgresPlanForeignModify has been modified so that if, in
>> addition to the above condition, the followings are satisfied, then the
>> ForeignScan and ModifyTable node will work that way.
>>
>> - There are no local BEFORE/AFTER triggers.
>
> The reason the check ignores INSTEAD OF triggers here is that INSTEAD
> OF trigger would prevent executing UPDATE/DELETE statement against a
> foreign tables at all, right?

I'm not sure that I understand your question correctly, but the reason
for that is because foreign tables cannot have INSTEAD OF triggers.

>> - In UPDATE it's safe to evaluate expressions to assign to the target
>> columns on the remote server.
>
> IIUC, in addition to target expressions, whole WHERE clause should be
> safe to be pushed-down. Though that is obviously required, but
> mentioning that in documents and source comments would help users and
> developers.

OK, I'll add the comments and documentation notes.

(I intentionaly didn't mention that because I think the comment for
postgresPlanForeignModify has already said the equivalent condition, ie,
"there are no local conditions", which means all conditions are executed
remotely.)

> We found that this patch speeds up DELETE case remarkably, as you
> describe above, but we saw only less than 2x speed on UPDATE cases.
> Do you have any numbers of UPDATE cases?

Here is the result for an UPDATE case.

>> On remote side:
>> postgres=# create table t (id serial primary key, inserted timestamp
>> default clock_timestamp(), data text);
>> CREATE TABLE
>> postgres=# insert into t(data) select random() from generate_series(0,
>> 99999);
>> INSERT 0 100000
>> postgres=# vacuum t;
>> VACUUM
>>
>> On local side:
>> postgres=# create foreign table ft (id integer, inserted timestamp, data
>> text) server myserver options (table_name 't');
>> CREATE FOREIGN TABLE

Unpatched:
postgres=# explain analyze verbose update ft set data = 'notsolongtext'
where id < 10000;
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
Update on public.ft (cost=100.00..147.38 rows=650 width=18) (actual
time=1463.944..1463.944 rows=0 loops=1)
Remote SQL: UPDATE public.t SET data = $2 WHERE ctid = $1
-> Foreign Scan on public.ft (cost=100.00..147.38 rows=650
width=18) (actual time=2.069..83.220 rows=9999 loops=1)
Output: id, inserted, 'notsolongtext'::text, ctid
Remote SQL: SELECT id, inserted, ctid FROM public.t WHERE ((id
< 10000)) FOR UPDATE
Planning time: 0.355 ms
Execution time: 1470.032 ms
(7 rows)

Patched:
postgres=# explain analyze verbose update ft set data = 'notsolongtext'
where id < 10000;
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------
Update on public.ft (cost=100.00..147.38 rows=650 width=18) (actual
time=0.005..0.005 rows=0 loops=1)
-> Foreign Scan on public.ft (cost=100.00..147.38 rows=650
width=18) (actual time=0.001..0.001 rows=0 loops=1)
Output: id, inserted, data, ctid
Remote SQL: UPDATE public.t SET data = 'notsolongtext'::text
WHERE ((id < 10000))
Planning time: 0.197 ms
Execution time: 61.519 ms
(6 rows)

I think that the precise effect of this optimization for DELETE/UPDATE
would depend on eg, data, queries (inc. w/ or w/o RETRUNING clauses) and
server/network performance. Could you tell me these information about
the UPDATE evaluation?

> Some more random thoughts:
>
> * Naming of new behavior
> You named this optimization "Direct Update", but I'm not sure that
> this is intuitive enough to express this behavior. I would like to
> hear opinions of native speakers.

+1

> * Macros for # of fdw_private elements
> In postgres_fdw.c you defined MaxFdwScanFdwPrivateLength and
> MinFdwScanFdwPrivateLength for determining the mode, but number of
> fdw_private elements is not a ranged value but an enum value (I mean
> that fdw_private holds 3 or 7, not 4~6, so Max and Min seems
> inappropriate for prefix. IMO those macros should be named so that
> the names represent the behavior, or define a macro to determine the
> mode, say IS_SHIPPABLE(foo).

OK, Will fix.

> * Comparison of Macros
> Comparison against MaxFdwScanFdwPrivateLength and
> MinFdwScanFdwPrivateLength should be == instead of >= or <= to detect
> unexpected value. Adding assert macro seems good too.

Will fix this too.

Thanks,

Best regards,
Etsuro Fujita

Shigeru Hanada wrote:
> * Naming of new behavior
> You named this optimization "Direct Update", but I'm not sure that
> this is intuitive enough to express this behavior. I would like to
> hear opinions of native speakers.

How about "batch foreign update" or "batch foreign modification"?
(Disclaimer: I'm not a native speaker either.)

Yours,
Laurenz Albe

On Fri, Jul 25, 2014 at 3:39 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Shigeru Hanada wrote:
>> * Naming of new behavior
>> You named this optimization "Direct Update", but I'm not sure that
>> this is intuitive enough to express this behavior. I would like to
>> hear opinions of native speakers.
>
> How about "batch foreign update" or "batch foreign modification"?
> (Disclaimer: I'm not a native speaker either.)

I think direct update sounds pretty good. "Batch" does not sound as
good to me, since it doesn't clearly describe what makes this patch
special as opposed to some other grouping of updates that happens to
produce a speedup.

Another term that might be used is "update pushdown", since we are
pushing the whole update to the remote server instead of having the
local server participate. Without looking at the patch, I don't have
a strong opinion on whether that's better than "direct update" in this
context.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2014/07/29 0:58), Robert Haas wrote:
> On Fri, Jul 25, 2014 at 3:39 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>> Shigeru Hanada wrote:
>>> * Naming of new behavior
>>> You named this optimization "Direct Update", but I'm not sure that
>>> this is intuitive enough to express this behavior. I would like to
>>> hear opinions of native speakers.
>>
>> How about "batch foreign update" or "batch foreign modification"?
>> (Disclaimer: I'm not a native speaker either.)
>
> I think direct update sounds pretty good. "Batch" does not sound as
> good to me, since it doesn't clearly describe what makes this patch
> special as opposed to some other grouping of updates that happens to
> produce a speedup.

I agree with Robert on that point.

> Another term that might be used is "update pushdown", since we are
> pushing the whole update to the remote server instead of having the
> local server participate. Without looking at the patch, I don't have
> a strong opinion on whether that's better than "direct update" in this
> context.

"Update Pushdown" is fine with me.

If there are no objections of others, I'll change the name from "Direct
Update" to "Update Pushdown".

Thanks,

Best regards,
Etsuro Fujita

Hi

2014-07-30 10:22 GMT+02:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:

> (2014/07/29 0:58), Robert Haas wrote:
>
>> On Fri, Jul 25, 2014 at 3:39 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>
>> wrote:
>>
>>> Shigeru Hanada wrote:
>>>
>>>> * Naming of new behavior
>>>> You named this optimization "Direct Update", but I'm not sure that
>>>> this is intuitive enough to express this behavior. I would like to
>>>> hear opinions of native speakers.
>>>>
>>>
>>> How about "batch foreign update" or "batch foreign modification"?
>>> (Disclaimer: I'm not a native speaker either.)
>>>
>>
>> I think direct update sounds pretty good. "Batch" does not sound as
>> good to me, since it doesn't clearly describe what makes this patch
>> special as opposed to some other grouping of updates that happens to
>> produce a speedup.
>>
>
> I agree with Robert on that point.
>
> Another term that might be used is "update pushdown", since we are
>> pushing the whole update to the remote server instead of having the
>> local server participate. Without looking at the patch, I don't have
>> a strong opinion on whether that's better than "direct update" in this
>> context.
>>
>
> "Update Pushdown" is fine with me.
>
> If there are no objections of others, I'll change the name from "Direct
> Update" to "Update Pushdown".
>

I like "Update Pushdown" - it is simple without other semantic

Regards

Pavel

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Hi Fujita-san,

Here is a new review result from Eitoku-san.

2014-07-25 16:30 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/07/24 18:30), Shigeru Hanada wrote:
> I'm not sure that I understand your question correctly, but the reason for
> that is because foreign tables cannot have INSTEAD OF triggers.

Now I see the reason, but then I worry (though it unlikely happens) a
case that new trigger type might be added in future. The code says
that "only BEFORE and AFTER triggers are unsafe for direct update",
but it would be more safe to code that "any trigger other than event
trigger is unsafe for direct update".

>> We found that this patch speeds up DELETE case remarkably, as you
>> describe above, but we saw only less than 2x speed on UPDATE cases.
>> Do you have any numbers of UPDATE cases?
>
>
> Here is the result for an UPDATE case.

Hmm, performance gain on UPDATE cases seems similar to our results,
except planning times. In your environment the patch reduces planning
time too, but we got longer planning times with your patch (in only
once in six trial, we got shorter planning time than average of
patched version). Could you try multiple times on your environment?

> I think that the precise effect of this optimization for DELETE/UPDATE would
> depend on eg, data, queries (inc. w/ or w/o RETRUNING clauses) and
> server/network performance. Could you tell me these information about the
> UPDATE evaluation?

I tried on a CentOS 6.5 on VMware on a Note PC with Core i3 1.17GHz,
2.0GB memory and single HDD, so the performance is poor.

The SQLs used for performance test are quite simple, update 10
thousands rows at a time, and repeat it for different section of the
table for six times. The definition of foreign table ft is same as
the one in your case.

EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >= 0
AND id < 10000;
EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
10000 AND id < 20000;
EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
20000 AND id < 30000;
EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
30000 AND id < 40000;
EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
40000 AND id < 50000;
EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
50000 AND id < 60000;

>> Some more random thoughts:
>>
>> * Naming of new behavior
>> You named this optimization "Direct Update", but I'm not sure that
>> this is intuitive enough to express this behavior. I would like to
>> hear opinions of native speakers.

Update push-down seems nice with according to others.

--
Shigeru HANADA

(2014/07/30 17:22), Etsuro Fujita wrote:
> (2014/07/29 0:58), Robert Haas wrote:
>> On Fri, Jul 25, 2014 at 3:39 AM, Albe Laurenz
>> <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>>> Shigeru Hanada wrote:
>>>> * Naming of new behavior
>>>> You named this optimization "Direct Update", but I'm not sure that
>>>> this is intuitive enough to express this behavior. I would like to
>>>> hear opinions of native speakers.
>>>
>>> How about "batch foreign update" or "batch foreign modification"?
>>> (Disclaimer: I'm not a native speaker either.)
>>
>> I think direct update sounds pretty good. "Batch" does not sound as
>> good to me, since it doesn't clearly describe what makes this patch
>> special as opposed to some other grouping of updates that happens to
>> produce a speedup.
>
> I agree with Robert on that point.
>
>> Another term that might be used is "update pushdown", since we are
>> pushing the whole update to the remote server instead of having the
>> local server participate. Without looking at the patch, I don't have
>> a strong opinion on whether that's better than "direct update" in this
>> context.
>
> "Update Pushdown" is fine with me.
>
> If there are no objections of others, I'll change the name from "Direct
> Update" to "Update Pushdown".

Done. (I've left deparseDirectUpdateSql/deparseDirectDeleteSql as-is,
though.)

Other changes:

* Address the comments from Eitoku-san.
* Add regression tests.
* Fix a bug, which fails to show the actual row counts in EXPLAIN
ANALYZE for UPDATE/DELETE without a RETURNING clause.
* Rebase to HEAD.

Please find attached an updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita

Hi Hanada-san,

Thank you for the answer.

(2014/08/04 19:36), Shigeru Hanada wrote:
> 2014-07-25 16:30 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> (2014/07/24 18:30), Shigeru Hanada wrote:
>> I'm not sure that I understand your question correctly, but the reason for
>> that is because foreign tables cannot have INSTEAD OF triggers.

> Now I see the reason, but then I worry (though it unlikely happens) a
> case that new trigger type might be added in future. The code says
> that "only BEFORE and AFTER triggers are unsafe for direct update",
> but it would be more safe to code that "any trigger other than event
> trigger is unsafe for direct update".

Yeah, I've revised the comment for that in the updated version of the
patch I sent in just now. Could you check it?

>>> We found that this patch speeds up DELETE case remarkably, as you
>>> describe above, but we saw only less than 2x speed on UPDATE cases.
>>> Do you have any numbers of UPDATE cases?

> Hmm, performance gain on UPDATE cases seems similar to our results,
> except planning times. In your environment the patch reduces planning
> time too, but we got longer planning times with your patch (in only
> once in six trial, we got shorter planning time than average of
> patched version). Could you try multiple times on your environment?

No. Is the overhead so large that it cannot be ignored?

>> I think that the precise effect of this optimization for DELETE/UPDATE would
>> depend on eg, data, queries (inc. w/ or w/o RETRUNING clauses) and
>> server/network performance. Could you tell me these information about the
>> UPDATE evaluation?

> I tried on a CentOS 6.5 on VMware on a Note PC with Core i3 1.17GHz,
> 2.0GB memory and single HDD, so the performance is poor.
>
> The SQLs used for performance test are quite simple, update 10
> thousands rows at a time, and repeat it for different section of the
> table for six times. The definition of foreign table ft is same as
> the one in your case.
>
> EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >= 0
> AND id < 10000;
> EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
> 10000 AND id < 20000;
> EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
> 20000 AND id < 30000;
> EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
> 30000 AND id < 40000;
> EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
> 40000 AND id < 50000;
> EXPLAIN ANALYZE VERBOSE UPDATe ft SET data = 'abcdefg' WHERE id >=
> 50000 AND id < 60000;

OK I also will evaluate the performance under the same workloads.

>>> Some more random thoughts:
>>>
>>> * Naming of new behavior
>>> You named this optimization "Direct Update", but I'm not sure that
>>> this is intuitive enough to express this behavior. I would like to
>>> hear opinions of native speakers.

> Update push-down seems nice with according to others.

The name has been changed in the updated version.

Thanks,

Best regards,
Etsuro Fujita

Hi Fujita-san,

Issues addressed by Eitoku-san were fixed properly, but he found a bug
and a possible enhancement in the v2 patch.

* push-down check misses delete triggers
update_is_pushdown_safe() seems to have a bug that it misses the
existence of row-level delete trigger. DELETE statement executed
against a foreign table which has row-level delete trigger is pushed
down to remote, and consequently no row-level delete trigger is fired.

* further optimization
Is there any chance to consider further optimization by passing the
operation type (UPDATE|DELETE) of undergoing statement to
update_is_pushdown_safe()? It seems safe to push down UPDATE
statement when the target foreign table has no update trigger even it
has a delete trigger (of course the opposite combination would be also
fine).

* Documentation
The requirement of pushing down UPDATE/DELETE statements would not be
easy to understand for non-expert users, so it seems that there is a
room to enhance documentation. An idea is to define which expression
is safe to send to remote first (it might need to mention the
difference of semantics), and refer the definition from the place
describing the requirement of pushing-down for SELECT, UPDATE and
DELETE.

2014-08-04 20:30 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/07/30 17:22), Etsuro Fujita wrote:
>>
>> (2014/07/29 0:58), Robert Haas wrote:
>>>
>>> On Fri, Jul 25, 2014 at 3:39 AM, Albe Laurenz
>>> <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>>>>
>>>> Shigeru Hanada wrote:
>>>>>
>>>>> * Naming of new behavior
>>>>> You named this optimization "Direct Update", but I'm not sure that
>>>>> this is intuitive enough to express this behavior. I would like to
>>>>> hear opinions of native speakers.
>>>>
>>>>
>>>> How about "batch foreign update" or "batch foreign modification"?
>>>> (Disclaimer: I'm not a native speaker either.)
>>>
>>>
>>> I think direct update sounds pretty good. "Batch" does not sound as
>>> good to me, since it doesn't clearly describe what makes this patch
>>> special as opposed to some other grouping of updates that happens to
>>> produce a speedup.
>>
>>
>> I agree with Robert on that point.
>>
>>> Another term that might be used is "update pushdown", since we are
>>> pushing the whole update to the remote server instead of having the
>>> local server participate. Without looking at the patch, I don't have
>>> a strong opinion on whether that's better than "direct update" in this
>>> context.
>>
>>
>> "Update Pushdown" is fine with me.
>>
>> If there are no objections of others, I'll change the name from "Direct
>> Update" to "Update Pushdown".
>
>
> Done. (I've left deparseDirectUpdateSql/deparseDirectDeleteSql as-is,
> though.)
>
> Other changes:
>
> * Address the comments from Eitoku-san.
> * Add regression tests.
> * Fix a bug, which fails to show the actual row counts in EXPLAIN ANALYZE
> for UPDATE/DELETE without a RETURNING clause.
> * Rebase to HEAD.
>
> Please find attached an updated version of the patch.
>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita

--
Shigeru HANADA

(2014/08/12 18:34), Shigeru Hanada wrote:
> Issues addressed by Eitoku-san were fixed properly, but he found a bug
> and a possible enhancement in the v2 patch.

Thank you for the review, Hanada-san and Eitoku-san!

> * push-down check misses delete triggers
> update_is_pushdown_safe() seems to have a bug that it misses the
> existence of row-level delete trigger. DELETE statement executed
> against a foreign table which has row-level delete trigger is pushed
> down to remote, and consequently no row-level delete trigger is fired.

Ah, I noticed that the current code for that is not correct. Will fix.

> * further optimization
> Is there any chance to consider further optimization by passing the
> operation type (UPDATE|DELETE) of undergoing statement to
> update_is_pushdown_safe()? It seems safe to push down UPDATE
> statement when the target foreign table has no update trigger even it
> has a delete trigger (of course the opposite combination would be also
> fine).

Good idea! Will improve that too.

> * Documentation
> The requirement of pushing down UPDATE/DELETE statements would not be
> easy to understand for non-expert users, so it seems that there is a
> room to enhance documentation. An idea is to define which expression
> is safe to send to remote first (it might need to mention the
> difference of semantics), and refer the definition from the place
> describing the requirement of pushing-down for SELECT, UPDATE and
> DELETE.

Yeah, I also think that it would not necessarily easy for the users to
understand which expression is safe to send. So I agree with that
enhancement, but ISTM that it would be better to do that as a separate
patch.

Thanks,

Best regards,
Etsuro Fujita

Etsuro Fujita wrote:
> Done. (I've left deparseDirectUpdateSql/deparseDirectDeleteSql as-is,
> though.)
>
> Other changes:
>
> * Address the comments from Eitoku-san.
> * Add regression tests.
> * Fix a bug, which fails to show the actual row counts in EXPLAIN
> ANALYZE for UPDATE/DELETE without a RETURNING clause.
> * Rebase to HEAD.
>
> Please find attached an updated version of the patch.

Here is my review:

The patch Applies fine, Builds without warning and passes make Check,
so the ABC of patch reviewing is fine.

I played with it, and apart from Hanada's comments I have found the following:

test=> EXPLAIN (ANALYZE, VERBOSE) UPDATE rtest SET val=NULL WHERE id > 3;
QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------
Update on laurenz.rtest (cost=100.00..14134.40 rows=299970 width=10) (actual time=0.005..0.005 rows=0 loops=1)
-> Foreign Scan on laurenz.rtest (cost=100.00..14134.40 rows=299970 width=10) (actual time=0.002..0.002 rows=299997 loops=1)
Output: id, val, ctid
Remote SQL: UPDATE laurenz.test SET val = NULL::text WHERE ((id > 3))
Planning time: 0.179 ms
Execution time: 3706.919 ms
(6 rows)

Time: 3708.272 ms

The "actual time" readings are surprising.
Shouldn't these similar to the actual execution time, since most of the time is spent
in the foreign scan node?

Reading the code, I noticed that the pushed down UPDATE or DELETE statement is executed
during postgresBeginForeignScan rather than during postgresIterateForeignScan.
It probably does not matter, but is there a reason to do it different from the normal scan?

It is not expected that postgresReScanForeignScan is called when the UPDATE/DELETE
is pushed down, right? Maybe it would make sense to add an assertion for that.

I ran a simple performance test and found that performance is improved as expected;
updating 100000 rows took 1000 rather than 8000 ms, and DELETING the same amount
took 200 instead of 6500 ms.

Yours,
Laurenz Albe

(2014/08/25 21:58), Albe Laurenz wrote:
> Here is my review:

Thank you for the review!

> I played with it, and apart from Hanada's comments I have found the following:
>
> test=> EXPLAIN (ANALYZE, VERBOSE) UPDATE rtest SET val=NULL WHERE id > 3;
> QUERY PLAN
> ----------------------------------------------------------------------------------------------------------------------------------
> Update on laurenz.rtest (cost=100.00..14134.40 rows=299970 width=10) (actual time=0.005..0.005 rows=0 loops=1)
> -> Foreign Scan on laurenz.rtest (cost=100.00..14134.40 rows=299970 width=10) (actual time=0.002..0.002 rows=299997 loops=1)
> Output: id, val, ctid
> Remote SQL: UPDATE laurenz.test SET val = NULL::text WHERE ((id > 3))
> Planning time: 0.179 ms
> Execution time: 3706.919 ms
> (6 rows)
>
> Time: 3708.272 ms
>
> The "actual time" readings are surprising.
> Shouldn't these similar to the actual execution time, since most of the time is spent
> in the foreign scan node?

I was also thinkng that this is confusing to the users. I think this is
because the patch executes the UPDATE/DELETE statement during
postgresBeginForeignScan, not postgresIterateForeignScan, as you
mentioned below:

> Reading the code, I noticed that the pushed down UPDATE or DELETE statement is executed
> during postgresBeginForeignScan rather than during postgresIterateForeignScan.
> It probably does not matter, but is there a reason to do it different from the normal scan?

I'll modify the patch so as to execute the statement during
postgresIterateForeignScan.

> It is not expected that postgresReScanForeignScan is called when the UPDATE/DELETE
> is pushed down, right? Maybe it would make sense to add an assertion for that.

IIUC, that is right. As ModifyTable doesn't support rescan currently,
postgresReScanForeignScan needn't to be called in the update pushdown
case. The assertion is a good idea. I'll add it.

Thanks,

Best regards,
Etsuro Fujita

On Mon, Aug 25, 2014 at 8:58 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
> Reading the code, I noticed that the pushed down UPDATE or DELETE statement is executed
> during postgresBeginForeignScan rather than during postgresIterateForeignScan.
> It probably does not matter, but is there a reason to do it different from the normal scan?

Hmm, I'm worried that may be an API contract violation. ISTM that we
might initialize nodes that we never read from - they can show up in
the EXPLAIN-plan as (never executed) - and things that aren't executed
shouldn't do work, especially work that permanently modifies data.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Aug 25, 2014 at 8:58 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>> Reading the code, I noticed that the pushed down UPDATE or DELETE statement is executed
>> during postgresBeginForeignScan rather than during postgresIterateForeignScan.
>> It probably does not matter, but is there a reason to do it different from the normal scan?

> Hmm, I'm worried that may be an API contract violation.

Indeed it is. You could get away with it if you check the
EXEC_FLAG_EXPLAIN_ONLY flag before doing anything with visible
side-effects, but it's still pretty ugly.

regards, tom lane

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Hmm, I'm worried that may be an API contract violation.

> Indeed it is. You could get away with it if you check the
> EXEC_FLAG_EXPLAIN_ONLY flag before doing anything with visible
> side-effects, but it's still pretty ugly.

Actually, there's another problem there. What of UPDATE or DELETE with a
LIMIT clause, which is something that seems to be coming down the pike:
https://commitfest.postgresql.org/action/patch_view?id=1550

regards, tom lane

(2014/08/27 22:56), Robert Haas wrote:
> On Mon, Aug 25, 2014 at 8:58 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>> Reading the code, I noticed that the pushed down UPDATE or DELETE statement is executed
>> during postgresBeginForeignScan rather than during postgresIterateForeignScan.
>> It probably does not matter, but is there a reason to do it different from the normal scan?
>
> Hmm, I'm worried that may be an API contract violation.

Will fix.

Thanks,

Best regards,
Etsuro Fujita

(2014/08/27 23:05), Tom Lane wrote:
> I wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Hmm, I'm worried that may be an API contract violation.

> Actually, there's another problem there. What of UPDATE or DELETE with a
> LIMIT clause, which is something that seems to be coming down the pike:
> https://commitfest.postgresql.org/action/patch_view?id=1550

I'd like to try to extend the functionality so as to push
UPDATE/DELETE-with-LIMIT down into the remote server if it's safe. But
I don't yet know if it's possible, because I started looking into the
UPDATE/DELETE-with-LIMIT patch just now ...

Thanks,

Best regards,
Etsuro Fujita

(2014/08/13 12:40), Etsuro Fujita wrote:
> (2014/08/12 18:34), Shigeru Hanada wrote:
>> Issues addressed by Eitoku-san were fixed properly, but he found a bug
>> and a possible enhancement in the v2 patch.

>> * push-down check misses delete triggers
>> update_is_pushdown_safe() seems to have a bug that it misses the
>> existence of row-level delete trigger. DELETE statement executed
>> against a foreign table which has row-level delete trigger is pushed
>> down to remote, and consequently no row-level delete trigger is fired.
>
> Ah, I noticed that the current code for that is not correct. Will fix.

Done.

>> * further optimization
>> Is there any chance to consider further optimization by passing the
>> operation type (UPDATE|DELETE) of undergoing statement to
>> update_is_pushdown_safe()? It seems safe to push down UPDATE
>> statement when the target foreign table has no update trigger even it
>> has a delete trigger (of course the opposite combination would be also
>> fine).
>
> Good idea! Will improve that too.

Done.

>> * Documentation
>> The requirement of pushing down UPDATE/DELETE statements would not be
>> easy to understand for non-expert users, so it seems that there is a
>> room to enhance documentation. An idea is to define which expression
>> is safe to send to remote first (it might need to mention the
>> difference of semantics), and refer the definition from the place
>> describing the requirement of pushing-down for SELECT, UPDATE and
>> DELETE.
>
> Yeah, I also think that it would not necessarily easy for the users to
> understand which expression is safe to send. So I agree with that
> enhancement, but ISTM that it would be better to do that as a separate
> patch.

As above, I'd like to leave this as another patch.

Please find attached the updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita

(2014/08/26 12:20), Etsuro Fujita wrote:
> (2014/08/25 21:58), Albe Laurenz wrote:
>> I played with it, and apart from Hanada's comments I have found the
>> following:
>>
>> test=> EXPLAIN (ANALYZE, VERBOSE) UPDATE rtest SET val=NULL WHERE id > 3;
>> QUERY PLAN
>> ----------------------------------------------------------------------------------------------------------------------------------
>>
>> Update on laurenz.rtest (cost=100.00..14134.40 rows=299970
>> width=10) (actual time=0.005..0.005 rows=0 loops=1)
>> -> Foreign Scan on laurenz.rtest (cost=100.00..14134.40
>> rows=299970 width=10) (actual time=0.002..0.002 rows=299997 loops=1)
>> Output: id, val, ctid
>> Remote SQL: UPDATE laurenz.test SET val = NULL::text WHERE
>> ((id > 3))
>> Planning time: 0.179 ms
>> Execution time: 3706.919 ms
>> (6 rows)
>>
>> Time: 3708.272 ms
>>
>> The "actual time" readings are surprising.
>> Shouldn't these similar to the actual execution time, since most of
>> the time is spent
>> in the foreign scan node?
>
> I was also thinkng that this is confusing to the users. I think this is
> because the patch executes the UPDATE/DELETE statement during
> postgresBeginForeignScan, not postgresIterateForeignScan, as you
> mentioned below:
>
>> Reading the code, I noticed that the pushed down UPDATE or DELETE
>> statement is executed
>> during postgresBeginForeignScan rather than during
>> postgresIterateForeignScan.

> I'll modify the patch so as to execute the statement during
> postgresIterateForeignScan.

Done.

>> It is not expected that postgresReScanForeignScan is called when the
>> UPDATE/DELETE
>> is pushed down, right? Maybe it would make sense to add an assertion
>> for that.
>
> IIUC, that is right. As ModifyTable doesn't support rescan currently,
> postgresReScanForeignScan needn't to be called in the update pushdown
> case. The assertion is a good idea. I'll add it.

Done.

You can find the updated version of the patch at

http://www.postgresql.org/message-id/53FFFA50.6020007@lab.ntt.co.jp

Thanks,

Best regards,
Etsuro Fujita

I confirmed performance gain accomplished by this patch.

This patch makes update queries ~50x faster, and even hit-miss update
is 3x faster than original. Of course benefit is only for queries
whose qualifiers are enough simple so that they can be pushied down
fully, but this improvement is remarkable.

This patch avoids 1) SELECT for determining target rows, and 2)
repeated per-row UPDATE/DELETE in particular situation, so I assumed
that the gain is larger for bulk update, and it's true indeed, but in
fact even hit-miss update (0 row affected) become faster enough. This
would come from the omission of SELECT preceding repeated
UPDATE/DELETE.

I was little worried about overhead in planning phase, but fluctuation
was less than 1ms, so it's negligible.

Measurement Result
==================

Note: numbers below are "execution time" of EXPLAIN ANALYZE, and
average of five runs
--------------+-------------+-----------+--------
rows affected | original | patched | gain
--------------+-------------+-----------+--------
0 | 4.841 | 1.548 | 3.13x
1 | 6.944 | 1.793 | 3.87x
100 | 174.420 | 5.167 | 33.76x
10,000 | 8,215.551 | 163.832 | 50.15x
100,000 | 78,135.905 | 1,595.739 | 48.97x
200,000 | 179,784.928 | 4,305.856 | 41.75x
--------------+-------------+-----------+--------

Measurement procedure
=====================

[Local side]
1) Create foreign table which refers pgbench_accounts on the remote side

[Remote side]
2) pgbench -i -s 100
3) Execute ANALYZE
4) Restart PostgreSQL to clear shared buffers

[Local side]
5) Execute ANALYZE against foreign table
6) Execute UPDATE SQL against foreign table once for warm the cache
7) Execute UPDATE SQL against foreign table five times

Test SQL for 10000-rows cas is below, only aid condition is changed
according to measurement variation.

EXPLAIN ANALYZE VERBOSE UPDATE ft_pgbench_accounts SET bid=bid+1,
abalance=abalance+1, filler='update test' WHERE aid<=10000;

2014-08-29 12:59 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/08/26 12:20), Etsuro Fujita wrote:
>>
>> (2014/08/25 21:58), Albe Laurenz wrote:
>>>
>>> I played with it, and apart from Hanada's comments I have found the
>>> following:
>>>
>>> test=> EXPLAIN (ANALYZE, VERBOSE) UPDATE rtest SET val=NULL WHERE id > 3;
>>> QUERY PLAN
>>>
>>> ----------------------------------------------------------------------------------------------------------------------------------
>>>
>>> Update on laurenz.rtest (cost=100.00..14134.40 rows=299970
>>> width=10) (actual time=0.005..0.005 rows=0 loops=1)
>>> -> Foreign Scan on laurenz.rtest (cost=100.00..14134.40
>>> rows=299970 width=10) (actual time=0.002..0.002 rows=299997 loops=1)
>>> Output: id, val, ctid
>>> Remote SQL: UPDATE laurenz.test SET val = NULL::text WHERE
>>> ((id > 3))
>>> Planning time: 0.179 ms
>>> Execution time: 3706.919 ms
>>> (6 rows)
>>>
>>> Time: 3708.272 ms
>>>
>>> The "actual time" readings are surprising.
>>> Shouldn't these similar to the actual execution time, since most of
>>> the time is spent
>>> in the foreign scan node?
>>
>>
>> I was also thinkng that this is confusing to the users. I think this is
>> because the patch executes the UPDATE/DELETE statement during
>> postgresBeginForeignScan, not postgresIterateForeignScan, as you
>> mentioned below:
>>
>>> Reading the code, I noticed that the pushed down UPDATE or DELETE
>>> statement is executed
>>> during postgresBeginForeignScan rather than during
>>> postgresIterateForeignScan.
>
>
>> I'll modify the patch so as to execute the statement during
>> postgresIterateForeignScan.
>
>
> Done.
>
>
>>> It is not expected that postgresReScanForeignScan is called when the
>>> UPDATE/DELETE
>>> is pushed down, right? Maybe it would make sense to add an assertion
>>> for that.
>>
>>
>> IIUC, that is right. As ModifyTable doesn't support rescan currently,
>> postgresReScanForeignScan needn't to be called in the update pushdown
>> case. The assertion is a good idea. I'll add it.
>
>
> Done.
>
> You can find the updated version of the patch at
>
> http://www.postgresql.org/message-id/53FFFA50.6020007@lab.ntt.co.jp
>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita

--
Shigeru HANADA

Etsuro Fujita wrote:
> Please find attached the updated version of the patch.

I gave it a spin and could not find any undesirable behaviour, and the
output of EXPLAIN ANALYZE looks like I'd expect.

I noticed that you use the list length of fdw_private to check if
the UPDATE or DELETE is pushed down to the remote server or not.

While this works fine, I wonder if it wouldn't be better to have some
explicit flag in fdw_private for that purpose. Future modifications that
change the list length might easily overlook that it is used for this
purpose, thereby breaking the code.

Other than that it looks alright to me.

Yours,
Laurenz Albe

I wrote:
> I gave it a spin and could not find any undesirable behaviour, and the
> output of EXPLAIN ANALYZE looks like I'd expect.
>
> I noticed that you use the list length of fdw_private to check if
> the UPDATE or DELETE is pushed down to the remote server or not.
>
> While this works fine, I wonder if it wouldn't be better to have some
> explicit flag in fdw_private for that purpose. Future modifications that
> change the list length might easily overlook that it is used for this
> purpose, thereby breaking the code.
>
> Other than that it looks alright to me.

Maybe I should have mentioned that I have set the patch to "Waiting for Author"
because I'd like to hear your opinion on that, but I'm prepared to set it
to "Ready for Committer" soon.

Yours,
Laurenz Albe

(2014/09/08 16:18), Albe Laurenz wrote:
> I wrote:
>> I gave it a spin and could not find any undesirable behaviour, and the
>> output of EXPLAIN ANALYZE looks like I'd expect.

Thank you for the review!

>> I noticed that you use the list length of fdw_private to check if
>> the UPDATE or DELETE is pushed down to the remote server or not.
>>
>> While this works fine, I wonder if it wouldn't be better to have some
>> explicit flag in fdw_private for that purpose. Future modifications that
>> change the list length might easily overlook that it is used for this
>> purpose, thereby breaking the code.

>> Other than that it looks alright to me.

> Maybe I should have mentioned that I have set the patch to "Waiting for Author"
> because I'd like to hear your opinion on that, but I'm prepared to set it
> to "Ready for Committer" soon.

I agree with you on that point. So, I've updated the patch to have the
explicit flag, as you proposed. Attached is the updated version of the
patch. In this version, I've also revised code and its comments a bit.

Sorry for the delay.

Best regards,
Etsuro Fujita

Etsuro Fujita wrote:
> I agree with you on that point. So, I've updated the patch to have the
> explicit flag, as you proposed. Attached is the updated version of the
> patch. In this version, I've also revised code and its comments a bit.

Thank you, I have set the patch to "Ready for Committer".

Yours,
Laurenz Albe

* Albe Laurenz (laurenz(dot)albe(at)wien(dot)gv(dot)at) wrote:
> Etsuro Fujita wrote:
> > I agree with you on that point. So, I've updated the patch to have the
> > explicit flag, as you proposed. Attached is the updated version of the
> > patch. In this version, I've also revised code and its comments a bit.
>
> Thank you, I have set the patch to "Ready for Committer".

I had a few minutes, so I started looking at this patch and I definitely
like where it's going. One concern which was brought up that I didn't
see completely addressed was around UPDATE/DELETE with LIMIT, which
seems to be making progress towards getting in. Presumably, we'd simply
disallow this optimization in that case, but we'll need a way to
identify that case..

I have to admit that, while I applaud the effort made to have this
change only be to postgres_fdw, I'm not sure that having the
update/delete happening during the Scan phase and then essentially
no-op'ing the ExecForeignUpdate/ExecForeignDelete is entirely in-line
with the defined API.

I would have thought we'd add a capability check function that says "can
this Modify be completely pushed down" and then have a way for that to
happen in ExecForeignUpdate/ExecForeignDelete. That means changes to
the existing API, of course, and if people feel that's unnecessary then
I'd suggest that we need to at least document that we're willing to
support these bulk operations happening on the remote siude during the
pre-Modify Scan and not during the ExecForeignUpdate/ExecForeignDelete.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I have to admit that, while I applaud the effort made to have this
> change only be to postgres_fdw, I'm not sure that having the
> update/delete happening during the Scan phase and then essentially
> no-op'ing the ExecForeignUpdate/ExecForeignDelete is entirely in-line
> with the defined API.

Yeah, I've started looking at this patch and that seemed like not
necessarily such a wise choice. I think it'd be better if the generated
plan tree had a different structure in this case. The existing approach
is an impressive hack but it's hard to call it anything but a hack.

I'm not sure offhand what the new plan tree ought to look like. We could
just generate a ForeignScan node, but that seems like rather a misnomer.
Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
be ForeignScan but with a flag field saying "hey this is really an update
(or a delete)". The main benefit I can think of right now is that the
EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
the only thing that ever looks at plan trees, so having an outright
misleading plan structure is likely to burn us down the line.

One advantage of getting the core code involved in the decision about
whether an update/delete can be pushed down is that FDW-independent checks
like whether there are relevant triggers could be implemented in the core
code, rather than having to duplicate them (and later maintain them) in
every FDW that wants to do this. OTOH, maybe the trigger issue is really
the only thing that could be shared, not sure. Stuff like "is there a
LIMIT" probably has to be in the FDW since some FDWs could support pushing
down LIMIT and others not.

regards, tom lane

Tom, all,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Yeah, I've started looking at this patch and that seemed like not
> necessarily such a wise choice. I think it'd be better if the generated
> plan tree had a different structure in this case. The existing approach
> is an impressive hack but it's hard to call it anything but a hack.

Agreed.

> I'm not sure offhand what the new plan tree ought to look like. We could
> just generate a ForeignScan node, but that seems like rather a misnomer.
> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
> be ForeignScan but with a flag field saying "hey this is really an update
> (or a delete)". The main benefit I can think of right now is that the
> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
> the only thing that ever looks at plan trees, so having an outright
> misleading plan structure is likely to burn us down the line.

My initial reaction is that a ForeignUpdate node makes the most sense.
That said, I've not invested any time trying to flesh that out, so take
my gut feeling for what it's worth.

> One advantage of getting the core code involved in the decision about
> whether an update/delete can be pushed down is that FDW-independent checks
> like whether there are relevant triggers could be implemented in the core
> code, rather than having to duplicate them (and later maintain them) in
> every FDW that wants to do this. OTOH, maybe the trigger issue is really
> the only thing that could be shared, not sure. Stuff like "is there a
> LIMIT" probably has to be in the FDW since some FDWs could support pushing
> down LIMIT and others not.

Yeah, agreed. I feel like we'll definitely want to support pushing down
LIMIT and it'll depend on the FDW as to if it can handle that or not
(and how it handles it, if it does). My thinking was actually that we'd
ask the FDW if it supports pushing down the UPDATE or DELETE statement,
as I suspect not all FDWs will want to answer that question the same way
for every case.

Thanks,

Stephen

Tom Lane wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> I have to admit that, while I applaud the effort made to have this
>> change only be to postgres_fdw, I'm not sure that having the
>> update/delete happening during the Scan phase and then essentially
>> no-op'ing the ExecForeignUpdate/ExecForeignDelete is entirely in-line
>> with the defined API.
>
> Yeah, I've started looking at this patch and that seemed like not
> necessarily such a wise choice. I think it'd be better if the generated
> plan tree had a different structure in this case. The existing approach
> is an impressive hack but it's hard to call it anything but a hack.

I guess that the idea is inspired by this comment on postgres_fdw.c:

* Note: currently, the plan tree generated for UPDATE/DELETE will always
* include a ForeignScan that retrieves ctids (using SELECT FOR UPDATE)
* and then the ModifyTable node will have to execute individual remote
* UPDATE/DELETE commands. If there are no local conditions or joins
* needed, it'd be better to let the scan node do UPDATE/DELETE RETURNING
* and then do nothing at ModifyTable. Room for future optimization ...

> I'm not sure offhand what the new plan tree ought to look like. We could
> just generate a ForeignScan node, but that seems like rather a misnomer.
> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
> be ForeignScan but with a flag field saying "hey this is really an update
> (or a delete)". The main benefit I can think of right now is that the
> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
> the only thing that ever looks at plan trees, so having an outright
> misleading plan structure is likely to burn us down the line.

I can understand these qualms.
I wonder if "ForeignUpdate" is such a good name though, since it would
surprise the uninitiate that in the regular (no push-down) case the
actual modification is *not* performed by ForeignUpdate.
So it should rather be a "ForeignModifyingScan", but I personally would
prefer a "has_side_effects" flag on ForeignScan.

> One advantage of getting the core code involved in the decision about
> whether an update/delete can be pushed down is that FDW-independent checks
> like whether there are relevant triggers could be implemented in the core
> code, rather than having to duplicate them (and later maintain them) in
> every FDW that wants to do this. OTOH, maybe the trigger issue is really
> the only thing that could be shared, not sure. Stuff like "is there a
> LIMIT" probably has to be in the FDW since some FDWs could support pushing
> down LIMIT and others not.

You are right, the gain would probably be limited.

Yours,
Laurenz Albe

(2014/09/12 16:03), Albe Laurenz wrote:
> Tom Lane wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>>> I have to admit that, while I applaud the effort made to have this
>>> change only be to postgres_fdw, I'm not sure that having the
>>> update/delete happening during the Scan phase and then essentially
>>> no-op'ing the ExecForeignUpdate/ExecForeignDelete is entirely in-line
>>> with the defined API.
>>
>> Yeah, I've started looking at this patch and that seemed like not
>> necessarily such a wise choice. I think it'd be better if the generated
>> plan tree had a different structure in this case. The existing approach
>> is an impressive hack but it's hard to call it anything but a hack.

Thank you for the review, Tom and Stephen!

> I guess that the idea is inspired by this comment on postgres_fdw.c:
>
> * Note: currently, the plan tree generated for UPDATE/DELETE will always
> * include a ForeignScan that retrieves ctids (using SELECT FOR UPDATE)
> * and then the ModifyTable node will have to execute individual remote
> * UPDATE/DELETE commands. If there are no local conditions or joins
> * needed, it'd be better to let the scan node do UPDATE/DELETE RETURNING
> * and then do nothing at ModifyTable. Room for future optimization ...

That's right. Thanks, Laurenz!

And in addition to that, I've created the patch with the conscious aim
of not getting the core code involved, because I was thinking this is
just an optimization. But I have to admit I was conscious of that too much.

>> I'm not sure offhand what the new plan tree ought to look like. We could
>> just generate a ForeignScan node, but that seems like rather a misnomer.
>> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
>> be ForeignScan but with a flag field saying "hey this is really an update
>> (or a delete)". The main benefit I can think of right now is that the
>> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
>> the only thing that ever looks at plan trees, so having an outright
>> misleading plan structure is likely to burn us down the line.
>
> I can understand these qualms.
> I wonder if "ForeignUpdate" is such a good name though, since it would
> surprise the uninitiate that in the regular (no push-down) case the
> actual modification is *not* performed by ForeignUpdate.
> So it should rather be a "ForeignModifyingScan", but I personally would
> prefer a "has_side_effects" flag on ForeignScan.

+1 for improving the EXPLAIN output by inventing a plan tree with a
different structure in this case, in general.

>> One advantage of getting the core code involved in the decision about
>> whether an update/delete can be pushed down is that FDW-independent checks
>> like whether there are relevant triggers could be implemented in the core
>> code, rather than having to duplicate them (and later maintain them) in
>> every FDW that wants to do this.

Good idea!

>> Stuff like "is there a
>> LIMIT" probably has to be in the FDW since some FDWs could support pushing
>> down LIMIT and others not.

That's what I have in mind. I'll work on that at the next CF.

Thanks,

Best regards,
Etsuro Fujita

Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> writes:
> Tom Lane wrote:
>> I'm not sure offhand what the new plan tree ought to look like. We could
>> just generate a ForeignScan node, but that seems like rather a misnomer.
>> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
>> be ForeignScan but with a flag field saying "hey this is really an update
>> (or a delete)". The main benefit I can think of right now is that the
>> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
>> the only thing that ever looks at plan trees, so having an outright
>> misleading plan structure is likely to burn us down the line.

> I can understand these qualms.
> I wonder if "ForeignUpdate" is such a good name though, since it would
> surprise the uninitiate that in the regular (no push-down) case the
> actual modification is *not* performed by ForeignUpdate.
> So it should rather be a "ForeignModifyingScan", but I personally would
> prefer a "has_side_effects" flag on ForeignScan.

I was envisioning that the EXPLAIN output would look like

Foreign Scan on tab1
Remote SQL: SELECT ...

for the normal case, versus

Foreign Update on tab1
Remote SQL: UPDATE ...

for the pushed-down-update case (and similarly for DELETE). For a
non-optimized update it'd still be a ForeignScan underneath a ModifyTable.

As for the internal representation, I was thinking of adding a CmdType
field to struct ForeignScan, with currently only CMD_SELECT, CMD_UPDATE,
CMD_DELETE as allowed values, though possibly in future we'd think of a
reason to allow CMD_INSERT there. This is more or less isomorphic to your
"has_side_effects" flag, but it allows distinguishing UPDATE from DELETE
which might be useful.

The only thing that's bothering me about this concept is that I'm not
seeing how to scale it up to handling a pushed-down update on a join,
ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote.
Maybe it's silly to worry about that until join push-down is done;
but in that case I'd vote for postponing this whole patch until we
have join push-down.

regards, tom lane

(2014/09/13 0:13), Tom Lane wrote:
> Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> writes:
>> Tom Lane wrote:
>>> I'm not sure offhand what the new plan tree ought to look like. We could
>>> just generate a ForeignScan node, but that seems like rather a misnomer.
>>> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
>>> be ForeignScan but with a flag field saying "hey this is really an update
>>> (or a delete)". The main benefit I can think of right now is that the
>>> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
>>> the only thing that ever looks at plan trees, so having an outright
>>> misleading plan structure is likely to burn us down the line.

> I was envisioning that the EXPLAIN output would look like
>
> Foreign Scan on tab1
> Remote SQL: SELECT ...
>
> for the normal case, versus
>
> Foreign Update on tab1
> Remote SQL: UPDATE ...
>
> for the pushed-down-update case (and similarly for DELETE). For a
> non-optimized update it'd still be a ForeignScan underneath a ModifyTable.

> As for the internal representation, I was thinking of adding a CmdType
> field to struct ForeignScan, with currently only CMD_SELECT, CMD_UPDATE,
> CMD_DELETE as allowed values, though possibly in future we'd think of a
> reason to allow CMD_INSERT there.

+1

> The only thing that's bothering me about this concept is that I'm not
> seeing how to scale it up to handling a pushed-down update on a join,
> ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote.
> Maybe it's silly to worry about that until join push-down is done;
> but in that case I'd vote for postponing this whole patch until we
> have join push-down.

OK

Thanks,

PS: I'll help Hanada-san do the work if there is anything I can do.

Best regards,
Etsuro Fujita

On 2014/09/13 0:13, Tom Lane wrote:
> Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at> writes:
>> Tom Lane wrote:
>>> I'm not sure offhand what the new plan tree ought to look like. We could
>>> just generate a ForeignScan node, but that seems like rather a misnomer.
>>> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still
>>> be ForeignScan but with a flag field saying "hey this is really an update
>>> (or a delete)". The main benefit I can think of right now is that the
>>> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
>>> the only thing that ever looks at plan trees, so having an outright
>>> misleading plan structure is likely to burn us down the line.
>
>> I can understand these qualms.
>> I wonder if "ForeignUpdate" is such a good name though, since it would
>> surprise the uninitiate that in the regular (no push-down) case the
>> actual modification is *not* performed by ForeignUpdate.
>> So it should rather be a "ForeignModifyingScan", but I personally would
>> prefer a "has_side_effects" flag on ForeignScan.
>
> I was envisioning that the EXPLAIN output would look like
>
> Foreign Scan on tab1
> Remote SQL: SELECT ...
>
> for the normal case, versus
>
> Foreign Update on tab1
> Remote SQL: UPDATE ...
>
> for the pushed-down-update case (and similarly for DELETE). For a
> non-optimized update it'd still be a ForeignScan underneath a ModifyTable.
>
> As for the internal representation, I was thinking of adding a CmdType
> field to struct ForeignScan, with currently only CMD_SELECT, CMD_UPDATE,
> CMD_DELETE as allowed values, though possibly in future we'd think of a
> reason to allow CMD_INSERT there. This is more or less isomorphic to your
> "has_side_effects" flag, but it allows distinguishing UPDATE from DELETE
> which might be useful.
>
> The only thing that's bothering me about this concept is that I'm not
> seeing how to scale it up to handling a pushed-down update on a join,
> ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote.
> Maybe it's silly to worry about that until join push-down is done;
> but in that case I'd vote for postponing this whole patch until we
> have join push-down.

I'll re-add this to the final CF. And I'll update the patch.

Best regards,
Etsuro Fujita

On 2015/02/16 12:03, Etsuro Fujita wrote:
> I'll update the patch.

While updating the patch, I noticed that in the previous patch, there is
a bug in pushing down parameterized UPDATE/DELETE queries; generic plans
for such queries fail with a can't-happen error. I fixed the bug and
tried to add the regression tests that execute the generic plans, but I
couldn't because I can't figure out how to force generic plans. Is
there any way to do that?

Best regards,
Etsuro Fujita

Etsuro Fujita wrote:
> While updating the patch, I noticed that in the previous patch, there is
> a bug in pushing down parameterized UPDATE/DELETE queries; generic plans
> for such queries fail with a can't-happen error. I fixed the bug and
> tried to add the regression tests that execute the generic plans, but I
> couldn't because I can't figure out how to force generic plans. Is
> there any way to do that?

I don't know about a way to force it, but if you run the statement six
times, it will probably switch to a generic plan.

Yours,
Laurenz Albe

On 2015/03/04 16:58, Albe Laurenz wrote:
> Etsuro Fujita wrote:
>> While updating the patch, I noticed that in the previous patch, there is
>> a bug in pushing down parameterized UPDATE/DELETE queries; generic plans
>> for such queries fail with a can't-happen error. I fixed the bug and
>> tried to add the regression tests that execute the generic plans, but I
>> couldn't because I can't figure out how to force generic plans. Is
>> there any way to do that?
>
> I don't know about a way to force it, but if you run the statement six
> times, it will probably switch to a generic plan.

Yeah, I was just thinking running the statement six times in the
regression tests ...

Thanks!

Best regards,
Etsuro Fujita

On 2015/03/04 17:07, Etsuro Fujita wrote:
> On 2015/03/04 16:58, Albe Laurenz wrote:
>> Etsuro Fujita wrote:
>>> While updating the patch, I noticed that in the previous patch, there is
>>> a bug in pushing down parameterized UPDATE/DELETE queries; generic plans
>>> for such queries fail with a can't-happen error. I fixed the bug and
>>> tried to add the regression tests that execute the generic plans, but I
>>> couldn't because I can't figure out how to force generic plans. Is
>>> there any way to do that?
>>
>> I don't know about a way to force it, but if you run the statement six
>> times, it will probably switch to a generic plan.
>
> Yeah, I was just thinking running the statement six times in the
> regression tests ...

Here is an updated version. In this version, the bug has been fixed,
but any regression tests for that hasn't been added, because I'm not
sure that the above way is a good idea and don't have any other ideas.

The EXPLAIN output has also been improved as discussed in [1].

On top of this, I'll try to extend the join push-down patch to support a
pushed-down update on a join, though I'm still digesting the series of
patches.

Comments welcome.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/31942.1410534785@sss.pgh.pa.us

On 2015/03/05 21:08, Etsuro Fujita wrote:
> Here is an updated version.

> The EXPLAIN output has also been improved as discussed in [1].

I noticed that the EXPLAIN for a pushed-down update (delete) on
inheritance childs doubly displays "Foreign Update" ("Foreign Delete"),
one for ForeignScan and the other for ModifyTable. Here is an example:

postgres=# explain verbose update parent set c1 = c1;
QUERY PLAN
------------------------------------------------------------------------------
Update on public.parent (cost=0.00..364.54 rows=4819 width=10)
Update on public.parent
Foreign Update on public.ft1
Foreign Update on public.ft2
-> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
Output: parent.c1, parent.ctid
-> Foreign Update on public.ft1 (cost=100.00..182.27 rows=2409
width=10)
Remote SQL: UPDATE public.t1 SET c1 = c1
-> Foreign Update on public.ft2 (cost=100.00..182.27 rows=2409
width=10)
Remote SQL: UPDATE public.t2 SET c1 = c1
(10 rows)

Should we do something? Suggestions are welcome.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/31942.1410534785@sss.pgh.pa.us

On 16-04-2015 PM 07:50, Etsuro Fujita wrote:
>> The EXPLAIN output has also been improved as discussed in [1].
>
> I noticed that the EXPLAIN for a pushed-down update (delete) on inheritance
> childs doubly displays "Foreign Update" ("Foreign Delete"), one for
> ForeignScan and the other for ModifyTable. Here is an example:
>
> postgres=# explain verbose update parent set c1 = c1;
> QUERY PLAN
> ------------------------------------------------------------------------------
> Update on public.parent (cost=0.00..364.54 rows=4819 width=10)
> Update on public.parent
> Foreign Update on public.ft1
> Foreign Update on public.ft2
> -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
> Output: parent.c1, parent.ctid
> -> Foreign Update on public.ft1 (cost=100.00..182.27 rows=2409 width=10)
> Remote SQL: UPDATE public.t1 SET c1 = c1
> -> Foreign Update on public.ft2 (cost=100.00..182.27 rows=2409 width=10)
> Remote SQL: UPDATE public.t2 SET c1 = c1
> (10 rows)
>
> Should we do something? Suggestions are welcome.
>

From what I see in Tom's commit message[0] for FTI patch, this shouldn't be,
right?

To be specific, there should be "Foreign Scan" there as per the commit. Am I
missing something?

Thanks,
Amit

[1]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=cb1ca4d800621dcae67ca6c799006de99fa4f0a5

On 2015/04/16 19:57, Amit Langote wrote:
> On 16-04-2015 PM 07:50, Etsuro Fujita wrote:
>>> The EXPLAIN output has also been improved as discussed in [1].
>>
>> I noticed that the EXPLAIN for a pushed-down update (delete) on inheritance
>> childs doubly displays "Foreign Update" ("Foreign Delete"), one for
>> ForeignScan and the other for ModifyTable. Here is an example:
>>
>> postgres=# explain verbose update parent set c1 = c1;
>> QUERY PLAN
>> ------------------------------------------------------------------------------
>> Update on public.parent (cost=0.00..364.54 rows=4819 width=10)
>> Update on public.parent
>> Foreign Update on public.ft1
>> Foreign Update on public.ft2
>> -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
>> Output: parent.c1, parent.ctid
>> -> Foreign Update on public.ft1 (cost=100.00..182.27 rows=2409 width=10)
>> Remote SQL: UPDATE public.t1 SET c1 = c1
>> -> Foreign Update on public.ft2 (cost=100.00..182.27 rows=2409 width=10)
>> Remote SQL: UPDATE public.t2 SET c1 = c1
>> (10 rows)
>>
>> Should we do something? Suggestions are welcome.

>>From what I see in Tom's commit message[0] for FTI patch, this shouldn't be,
> right?
>
> To be specific, there should be "Foreign Scan" there as per the commit. Am I
> missing something?

As shown in the below example, this patch doesn't change the EXPLAIN
output for non-pushed-down update (delete) cases, but since we changed
the EXPLAIN output as discussed in [1], the patch doubly displays
"Foreign Update" ("Foreign Delete") for pushed-down update (delet) cases
like the above example.

postgres=# explain verbose update parent set c1 = trunc(random() * 9 +
1)::int;
QUERY PLAN
-------------------------------------------------------------------------------------------------------------
Update on public.parent (cost=0.00..452.06 rows=5461 width=6)
Update on public.parent
Foreign Update on public.ft1
Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
Foreign Update on public.ft2
Remote SQL: UPDATE public.t2 SET c1 = $2 WHERE ctid = $1
-> Seq Scan on public.parent (cost=0.00..0.01 rows=1 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, parent.ctid
-> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, ft1.ctid
Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
-> Foreign Scan on public.ft2 (cost=100.00..226.03 rows=2730 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, ft2.ctid
Remote SQL: SELECT ctid FROM public.t2 FOR UPDATE
(14 rows)

Best regards,
Etsuro Fujita

Fujita-san,

On 16-04-2015 PM 08:40, Etsuro Fujita wrote:
>> From what I see in Tom's commit message[0] for FTI patch, this shouldn't be,
>> right?
>>
>> To be specific, there should be "Foreign Scan" there as per the commit. Am I
>> missing something?
>
> As shown in the below example, this patch doesn't change the EXPLAIN
> output for non-pushed-down update (delete) cases, but since we changed
> the EXPLAIN output as discussed in [1], the patch doubly displays
> "Foreign Update" ("Foreign Delete") for pushed-down update (delet) cases
> like the above example.
>
> postgres=# explain verbose update parent set c1 = trunc(random() * 9 +
> 1)::int;
> QUERY PLAN
> -------------------------------------------------------------------------------------------------------------
> Update on public.parent (cost=0.00..452.06 rows=5461 width=6)
> Update on public.parent
> Foreign Update on public.ft1
> Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
> Foreign Update on public.ft2
> Remote SQL: UPDATE public.t2 SET c1 = $2 WHERE ctid = $1
> -> Seq Scan on public.parent (cost=0.00..0.01 rows=1 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, parent.ctid
> -> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, ft1.ctid
> Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
> -> Foreign Scan on public.ft2 (cost=100.00..226.03 rows=2730 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, ft2.ctid
> Remote SQL: SELECT ctid FROM public.t2 FOR UPDATE
> (14 rows)
>

I think I missed the point that you are talking about the result after the
patch for foreign udpate pushdown (which is the topic of this thread) has been
applied. Sorry about the noise.

By the way, one suggestion may be to attach a "(pushed down)" to the
ModifyTable's "Foreign Update". And in that case, there would be no mention of
corresponding scan node in the list below exactly because there would be none.

postgres=# explain verbose update parent set c1 = c1;
QUERY PLAN
------------------------------------------------------------------------------
Update on public.parent (cost=0.00..364.54 rows=4819 width=10)
Update on public.parent
Foreign Update (pushed down) on public.ft1
Foreign Update (pushed down) on public.ft2
-> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
Output: parent.c1, parent.ctid

Thoughts?

Thanks,
Amit

On 2015/04/17 10:23, Amit Langote wrote:
> By the way, one suggestion may be to attach a "(pushed down)" to the
> ModifyTable's "Foreign Update". And in that case, there would be no mention of
> corresponding scan node in the list below exactly because there would be none.
>
> postgres=# explain verbose update parent set c1 = c1;
> QUERY PLAN
> ------------------------------------------------------------------------------
> Update on public.parent (cost=0.00..364.54 rows=4819 width=10)
> Update on public.parent
> Foreign Update (pushed down) on public.ft1
> Foreign Update (pushed down) on public.ft2
> -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
> Output: parent.c1, parent.ctid

Thanks for the suggestion!

I'm not sure that that is a good idea because (1) that is contrary to
the reality (the update pushdown patch lets the ForeignScan nodes do
UPDATE/DELETE RETURNING and then do nothing at ModifyTable!) and because
(2) that might cause the problem of associating subplans' update
information with subplans' scan information, pointed out by Tom [1].

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/22505.1426986174@sss.pgh.pa.us

On 17-04-2015 PM 12:35, Etsuro Fujita wrote:
> On 2015/04/17 10:23, Amit Langote wrote:
>> By the way, one suggestion may be to attach a "(pushed down)" to the
>> ModifyTable's "Foreign Update". And in that case, there would be no mention of
>> corresponding scan node in the list below exactly because there would be none.
>>
>> postgres=# explain verbose update parent set c1 = c1;
>> QUERY PLAN
>> ------------------------------------------------------------------------------
>> Update on public.parent (cost=0.00..364.54 rows=4819 width=10)
>> Update on public.parent
>> Foreign Update (pushed down) on public.ft1
>> Foreign Update (pushed down) on public.ft2
>> -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
>> Output: parent.c1, parent.ctid
>
> Thanks for the suggestion!
>
> I'm not sure that that is a good idea because (1) that is contrary to
> the reality (the update pushdown patch lets the ForeignScan nodes do
> UPDATE/DELETE RETURNING and then do nothing at ModifyTable!) and because

Ah, the reality is exactly the reverse then. Thanks for your patience.

> (2) that might cause the problem of associating subplans' update
> information with subplans' scan information, pointed out by Tom [1].
>

Having realized how it really works now, my +1 to "Foreign Modifying Scan" for
cases of pushed down update as suggested by Albe Laurenz. I guess it would be
signaled by the proposed ForeignScan.CmdType being CMD_UPDATE / CMP_UPDATE
(/CMD_INSERT).

Thanks,
Amit

On 2015/04/17 13:16, Amit Langote wrote:
> On 17-04-2015 PM 12:35, Etsuro Fujita wrote:
>> (2) that might cause the problem of associating subplans' update
>> information with subplans' scan information, pointed out by Tom [1].

> Having realized how it really works now, my +1 to "Foreign Modifying Scan" for
> cases of pushed down update as suggested by Albe Laurenz. I guess it would be
> signaled by the proposed ForeignScan.CmdType being CMD_UPDATE / CMP_UPDATE
> (/CMD_INSERT).

Thanks for the opinion! I think that that is an idea. However, I'd
like to propose to rename "Foreign Update" ("Foreign Delete") of
ModifyTable simply to "Update" ("Delete") not only because (1) that
solves the duplication problem but also because (2) ISTM that is
consistent with the non-inherited updates in both of the
non-pushed-down-update case and the pushed-down-update case. Here are
examples for (2).

* Inherited and non-inherited updates for the non-pushed-down case:

postgres=# explain verbose update parent set c1 = trunc(random() * 9 +
1)::int;
QUERY PLAN
-------------------------------------------------------------------------------------------------------------
Update on public.parent (cost=0.00..452.06 rows=5461 width=6)
Update on public.parent
Update on public.ft1
Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
Update on public.ft2
Remote SQL: UPDATE public.t2 SET c1 = $2 WHERE ctid = $1
-> Seq Scan on public.parent (cost=0.00..0.01 rows=1 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, parent.ctid
-> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, ft1.ctid
Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
-> Foreign Scan on public.ft2 (cost=100.00..226.03 rows=2730 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, ft2.ctid
Remote SQL: SELECT ctid FROM public.t2 FOR UPDATE
(14 rows)

postgres=# explain verbose update ft1 set c1 = trunc(random() * 9 + 1)::int;
QUERY PLAN
------------------------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
-> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
Output: (trunc(((random() * '9'::double precision) +
'1'::double precision)))::integer, ctid
Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
(5 rows)

* Inherited and non-inherited updates for the pushed-down case:

postgres=# explain verbose update parent set c1 = c1 + 1;
QUERY PLAN
------------------------------------------------------------------------------
Update on public.parent (cost=0.00..376.59 rows=4819 width=10)
Update on public.parent
Update on public.ft1
Update on public.ft2
-> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
Output: (parent.c1 + 1), parent.ctid
-> Foreign Update on public.ft1 (cost=100.00..188.29 rows=2409
width=10)
Remote SQL: UPDATE public.t1 SET c1 = (c1 + 1)
-> Foreign Update on public.ft2 (cost=100.00..188.29 rows=2409
width=10)
Remote SQL: UPDATE public.t2 SET c1 = (c1 + 1)
(10 rows)

postgres=# explain verbose update ft1 set c1 = c1 + 1;
QUERY PLAN
------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..188.29 rows=2409 width=10)
-> Foreign Update on public.ft1 (cost=100.00..188.29 rows=2409
width=10)
Remote SQL: UPDATE public.t1 SET c1 = (c1 + 1)
(3 rows)

Comments are welcome.

Best regards,
Etsuro Fujita

Hi,

At Mon, 20 Apr 2015 16:40:52 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <5534AD84(dot)3020501(at)lab(dot)ntt(dot)co(dot)jp>
> On 2015/04/17 13:16, Amit Langote wrote:
> > On 17-04-2015 PM 12:35, Etsuro Fujita wrote:
> >> (2) that might cause the problem of associating subplans' update
> >> information with subplans' scan information, pointed out by Tom [1].
>
> > Having realized how it really works now, my +1 to "Foreign Modifying Scan" for
> > cases of pushed down update as suggested by Albe Laurenz. I guess it would be
> > signaled by the proposed ForeignScan.CmdType being CMD_UPDATE / CMP_UPDATE
> > (/CMD_INSERT).
>
> Thanks for the opinion! I think that that is an idea. However, I'd
> like to propose to rename "Foreign Update" ("Foreign Delete") of
> ModifyTable simply to "Update" ("Delete") not only because (1) that
> solves the duplication problem but also because (2) ISTM that is
> consistent with the non-inherited updates in both of the
> non-pushed-down-update case and the pushed-down-update case. Here are
> examples for (2).

Update node without "Foreign" that runs "Remote SQL" looks to me
somewhat unusual..

It seems to me that the problem is "Foreign Update"s for
ModifyTable that does nothing eventually. Even though I don't
understand this fully, especially what "Foreign Update" for
ModifyTable does when "Foreign Update" in place of "Foreign Scan"
finished substantial work, I think the ForeignUpdate nodes should
be removed during planning if it is really ineffective, or such
"Foreign Update"s shoud be renamed or provided with some
explaination in explain output if it does anything or unremovable
from some reason.

If removed it looks like,

| =# explain verbose update p set b = b + 1;
| QUERY PLAN
| ------------------------------------------------------------------------------
| Update on public.p (cost=0.00..360.08 rows=4311 width=14)
| Update on public.p
| -> Seq Scan on public.p (cost=0.00..0.00 rows=1 width=14)
| Output: p.a, (p.b + 1), p.ctid
| -> Foreign Update on public.ft1 (cost=100.00..180.04 rows=2155 width=14)
| Remote SQL: UPDATE public.t1 SET b = (b + 1)
| -> Foreign Update on public.ft2 (cost=100.00..180.04 rows=2155 width=14)
| Remote SQL: UPDATE public.t2 SET b = (b + 1)

regards,

> * Inherited and non-inherited updates for the non-pushed-down case:
>
> postgres=# explain verbose update parent set c1 = trunc(random() * 9 +
> 1)::int;
> QUERY PLAN
> -------------------------------------------------------------------------------------------------------------
> Update on public.parent (cost=0.00..452.06 rows=5461 width=6)
> Update on public.parent
> Update on public.ft1
> Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
> Update on public.ft2
> Remote SQL: UPDATE public.t2 SET c1 = $2 WHERE ctid = $1
> -> Seq Scan on public.parent (cost=0.00..0.01 rows=1 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, parent.ctid
> -> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, ft1.ctid
> Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
> -> Foreign Scan on public.ft2 (cost=100.00..226.03 rows=2730 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, ft2.ctid
> Remote SQL: SELECT ctid FROM public.t2 FOR UPDATE
> (14 rows)
>
> postgres=# explain verbose update ft1 set c1 = trunc(random() * 9 + 1)::int;
> QUERY PLAN
> ------------------------------------------------------------------------------------------------------
> Update on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
> Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
> -> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
> Output: (trunc(((random() * '9'::double precision) +
> '1'::double precision)))::integer, ctid
> Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
> (5 rows)
>
> * Inherited and non-inherited updates for the pushed-down case:
>
> postgres=# explain verbose update parent set c1 = c1 + 1;
> QUERY PLAN
> ------------------------------------------------------------------------------
> Update on public.parent (cost=0.00..376.59 rows=4819 width=10)
> Update on public.parent
> Update on public.ft1
> Update on public.ft2
> -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10)
> Output: (parent.c1 + 1), parent.ctid
> -> Foreign Update on public.ft1 (cost=100.00..188.29 rows=2409
> width=10)
> Remote SQL: UPDATE public.t1 SET c1 = (c1 + 1)
> -> Foreign Update on public.ft2 (cost=100.00..188.29 rows=2409
> width=10)
> Remote SQL: UPDATE public.t2 SET c1 = (c1 + 1)
> (10 rows)
>
> postgres=# explain verbose update ft1 set c1 = c1 + 1;
> QUERY PLAN
> ------------------------------------------------------------------------------
> Update on public.ft1 (cost=100.00..188.29 rows=2409 width=10)
> -> Foreign Update on public.ft1 (cost=100.00..188.29 rows=2409
> width=10)
> Remote SQL: UPDATE public.t1 SET c1 = (c1 + 1)
> (3 rows)
>
> Comments are welcome.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2015/04/21 10:07, Kyotaro HORIGUCHI wrote:
> At Mon, 20 Apr 2015 16:40:52 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <5534AD84(dot)3020501(at)lab(dot)ntt(dot)co(dot)jp>
>> However, I'd
>> like to propose to rename "Foreign Update" ("Foreign Delete") of
>> ModifyTable simply to "Update" ("Delete") not only because (1) that
>> solves the duplication problem but also because (2) ISTM that is
>> consistent with the non-inherited updates in both of the
>> non-pushed-down-update case and the pushed-down-update case. Here are
>> examples for (2).

> Update node without "Foreign" that runs "Remote SQL" looks to me
> somewhat unusual..

I think that has a similarity with the existing EXPLAIN outputs for
non-inherited non-pushed-down updates, as shown in the below exaple.

>> postgres=# explain verbose update ft1 set c1 = trunc(random() * 9 +
1)::int;
>> QUERY PLAN
>>
------------------------------------------------------------------------------------------------------
>> Update on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
>> Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
>> -> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730
width=6)
>> Output: (trunc(((random() * '9'::double precision) +
>> '1'::double precision)))::integer, ctid
>> Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
>> (5 rows)

> It seems to me that the problem is "Foreign Update"s for
> ModifyTable that does nothing eventually.

> I think the ForeignUpdate nodes should
> be removed during planning if it is really ineffective,

> If removed it looks like,
>
> | =# explain verbose update p set b = b + 1;
> | QUERY PLAN
> | ------------------------------------------------------------------------------
> | Update on public.p (cost=0.00..360.08 rows=4311 width=14)
> | Update on public.p
> | -> Seq Scan on public.p (cost=0.00..0.00 rows=1 width=14)
> | Output: p.a, (p.b + 1), p.ctid
> | -> Foreign Update on public.ft1 (cost=100.00..180.04 rows=2155 width=14)
> | Remote SQL: UPDATE public.t1 SET b = (b + 1)
> | -> Foreign Update on public.ft2 (cost=100.00..180.04 rows=2155 width=14)
> | Remote SQL: UPDATE public.t2 SET b = (b + 1)

On that point, I agree with Tom that that would cause the problem that
the user has to guess at which of the child plans goes with which target
relation of ModifyTable [1].

Thanks for the comments!

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/22505.1426986174@sss.pgh.pa.us

Hi, thank you. My understanding became a bit clearer.

At Tue, 21 Apr 2015 15:35:41 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <5535EFBD(dot)8030006(at)lab(dot)ntt(dot)co(dot)jp>
> On 2015/04/21 10:07, Kyotaro HORIGUCHI wrote:
> > At Mon, 20 Apr 2015 16:40:52 +0900, Etsuro Fujita
> > <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in
> > <5534AD84(dot)3020501(at)lab(dot)ntt(dot)co(dot)jp>
> >> However, I'd
> >> like to propose to rename "Foreign Update" ("Foreign Delete") of
> >> ModifyTable simply to "Update" ("Delete") not only because (1) that
> >> solves the duplication problem but also because (2) ISTM that is
> >> consistent with the non-inherited updates in both of the
> >> non-pushed-down-update case and the pushed-down-update case. Here are
> >> examples for (2).
>
> > Update node without "Foreign" that runs "Remote SQL" looks to me
> > somewhat unusual..
>
> I think that has a similarity with the existing EXPLAIN outputs for
> non-inherited non-pushed-down updates, as shown in the below exaple.
>
> >> postgres=# explain verbose update ft1 set c1 = trunc(random() * 9 +
> >> 1)::int;
> >> QUERY PLAN
> >>
> ------------------------------------------------------------------------------------------------------
> >> Update on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
> >> Remote SQL: UPDATE public.t1 SET c1 = $2 WHERE ctid = $1
> >> -> Foreign Scan on public.ft1 (cost=100.00..226.03 rows=2730 width=6)
> >> Output: (trunc(((random() * '9'::double precision) +
> >> '1'::double precision)))::integer, ctid
> >> Remote SQL: SELECT ctid FROM public.t1 FOR UPDATE
> >> (5 rows)

Mmm.. It also looks confusing which needs to be fixed. Now
foreign tables are updated in two ways. One is ModifyTable on
foreign relation and the another is ForeignScan node of update
operation. Though I think that I understand the path to this
form, but I suppose they should confluent into one type of node,
perhaps ForegnScan node. Even if it is hardly archievable for
now, explain representation should be uniform.

Making ModifyTable on foreign relation have the representation
"Foreign Update", the explain results of the queries modifying
foreign tables are looks like,

Foreign Update on public.ft1 (...)
Remote SQL: UPDATE public.t1 ....
-> Foreign Scan on public.ft1...

Foreign Update on public.ft1 (...
Foreign Update on public.ft1 (...
Remote SQL: ...

If Foreign Update has only one internal representation, the two
same Foreign Updates are (ideally) easily eliminated during
planning and explain would naturally shows the following result.

Foreign Update on public.ft1 (...
Remote SQL: ...

But if not as is currently so, printing the result needs a bit
complicated calculation.

> > | Update on public.p (cost=0.00..360.08 rows=4311 width=14)
> > | Update on public.p
> > | -> Seq Scan on public.p (cost=0.00..0.00 rows=1 width=14)
> > | Output: p.a, (p.b + 1), p.ctid
> > | -> Foreign Update on public.ft1 (cost=100.00..180.04 rows=2155
> > | -> width=14)
> > | Remote SQL: UPDATE public.t1 SET b = (b + 1)
> > | -> Foreign Update on public.ft2 (cost=100.00..180.04 rows=2155
> > | -> width=14)
> > | Remote SQL: UPDATE public.t2 SET b = (b + 1)
>
> On that point, I agree with Tom that that would cause the problem that
> the user has to guess at which of the child plans goes with which
> target relation of ModifyTable [1].
>
> [1]
> http://www.postgresql.org/message-id/22505.1426986174@sss.pgh.pa.us

Yeah, that seems to make the plan to be understood
clerer. Combining Tom's suggestion and my suggestion together
would result in the following output of explain.

Update on public.p (cost=0.00..360.08 rows=4311 width=14)
Update on public.p
-> Seq Scan on public.p (cost=0.00..0.00 rows=1 width=14)
Output: p.a, (p.b + 1), p.ctid
Foreign Update on public.ft1 (cost=100.00..180.04 rows=2155 width=14)
Remote SQL: UPDATE public.t1 SET b = (b + 1)
Foreign Update on public.ft2 (cost=100.00..180.04 rows=2155 width=14)
Remote SQL: UPDATE public.t2 SET b = (b + 1)

And when not pushed down it would look like,

Update on public.p (cost=0.00..360.08 rows=4311 width=14)
Update on public.p
-> Seq Scan on public.p (cost=0.00..0.00 rows=1 width=14)
Output: p.a, (p.b + 1), p.ctid
Foreign Update on public.ft1 (cost=100.00..180.04 rows=2155 width=14)
Remote SQL: UPDATE public.t1 SET b = $2 WHERE ctid = $1
-> Foreign Scan on public.ft1 (cost=....)
Output: a, b, ctid
Remote SQL: SELECT a, ctid FROM public.t1 FOR UPDATE
Foreign Update on public.ft2 (cost=100.00..180.04 rows=2155 width=14)
Remote SQL: UPDATE public.t2 SET b = (b + 1)
-> Foreign Scan on public.ft2 (cost=....)
Output: a, b, ctid
Remote SQL: SELECT a, ctid FROM public.t2 FOR UPDATE

These looks quite reasonable *for me* :)

Of course, the same discussion is applicable on Foreign Delete.

What do you think about this?

Any further thoughts?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Etsuro,

* Etsuro Fujita (fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp) wrote:
> Here is an updated version. In this version, the bug has been
> fixed, but any regression tests for that hasn't been added, because
> I'm not sure that the above way is a good idea and don't have any
> other ideas.
>
> The EXPLAIN output has also been improved as discussed in [1].

While the EXPLAIN output changed, the structure hasn't really changed
from what was discussed previously and there's not been any real
involvment from the core code in what's happening here.

Clearly, the documentation around how to use the FDW API hasn't changed
at all and there's been no additions to it for handling bulk work.
Everything here continues to be done inside of postgres_fdw, which
essentially ignores the prescribed "Update/Delete one tuple" interface
for ExecForeignUpdate/ExecForeignDelete.

I've spent the better part of the past two days trying to reason my way
around that while reviewing this patch and I haven't come out the other
side any happier with this approach than I was back in
20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)

There are other things that don't look right to me, such as what's going
on at the bottom of push_update_down(), but I don't think there's much
point going into it until we figure out what the core FDW API here
should look like. It might not be all that far from what we have now,
but I don't think we can just ignore the existing, documented, API.

Thanks!

Stephen

On 2015/05/13 0:55, Stephen Frost wrote:
> Etsuro,
>
> * Etsuro Fujita (fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp) wrote:
>> Here is an updated version. In this version, the bug has been
>> fixed, but any regression tests for that hasn't been added, because
>> I'm not sure that the above way is a good idea and don't have any
>> other ideas.
>>
>> The EXPLAIN output has also been improved as discussed in [1].
>
> While the EXPLAIN output changed, the structure hasn't really changed
> from what was discussed previously and there's not been any real
> involvment from the core code in what's happening here.
>
> Clearly, the documentation around how to use the FDW API hasn't changed
> at all and there's been no additions to it for handling bulk work.
> Everything here continues to be done inside of postgres_fdw, which
> essentially ignores the prescribed "Update/Delete one tuple" interface
> for ExecForeignUpdate/ExecForeignDelete.
>
> I've spent the better part of the past two days trying to reason my way
> around that while reviewing this patch and I haven't come out the other
> side any happier with this approach than I was back in
> 20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)
>
> There are other things that don't look right to me, such as what's going
> on at the bottom of push_update_down(), but I don't think there's much
> point going into it until we figure out what the core FDW API here
> should look like. It might not be all that far from what we have now,
> but I don't think we can just ignore the existing, documented, API.

OK, I'll try to introduce the core FDW API for this (and make changes to
the core code) to address your previous comments.

Thanks for taking the time to review the patch!

Best regards,
Etsuro Fujita

On 13 May 2015 at 04:10, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2015/05/13 0:55, Stephen Frost wrote:
>>
>> Etsuro,
>>
>> * Etsuro Fujita (fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp) wrote:
>>>
>>> Here is an updated version. In this version, the bug has been
>>> fixed, but any regression tests for that hasn't been added, because
>>> I'm not sure that the above way is a good idea and don't have any
>>> other ideas.
>>>
>>> The EXPLAIN output has also been improved as discussed in [1].
>>
>>
>> While the EXPLAIN output changed, the structure hasn't really changed
>> from what was discussed previously and there's not been any real
>> involvment from the core code in what's happening here.
>>
>> Clearly, the documentation around how to use the FDW API hasn't changed
>> at all and there's been no additions to it for handling bulk work.
>> Everything here continues to be done inside of postgres_fdw, which
>> essentially ignores the prescribed "Update/Delete one tuple" interface
>> for ExecForeignUpdate/ExecForeignDelete.
>>
>> I've spent the better part of the past two days trying to reason my way
>> around that while reviewing this patch and I haven't come out the other
>> side any happier with this approach than I was back in
>> 20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)
>>
>> There are other things that don't look right to me, such as what's going
>> on at the bottom of push_update_down(), but I don't think there's much
>> point going into it until we figure out what the core FDW API here
>> should look like. It might not be all that far from what we have now,
>> but I don't think we can just ignore the existing, documented, API.
>
>
> OK, I'll try to introduce the core FDW API for this (and make changes to the
> core code) to address your previous comments.
>
> Thanks for taking the time to review the patch!

Fujita-san,

I'm a bit behind in reading up on this, so maybe it's been covered
since, but is there a discussion of this API on another thread, or a
newer patch available?

Thanks

Thom

Hi Thom,

Thank you for paying attention to this!

On 2015/11/25 20:36, Thom Brown wrote:
> On 13 May 2015 at 04:10, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2015/05/13 0:55, Stephen Frost wrote:
>>> While the EXPLAIN output changed, the structure hasn't really changed
>>> from what was discussed previously and there's not been any real
>>> involvment from the core code in what's happening here.
>>>
>>> Clearly, the documentation around how to use the FDW API hasn't changed
>>> at all and there's been no additions to it for handling bulk work.
>>> Everything here continues to be done inside of postgres_fdw, which
>>> essentially ignores the prescribed "Update/Delete one tuple" interface
>>> for ExecForeignUpdate/ExecForeignDelete.
>>>
>>> I've spent the better part of the past two days trying to reason my way
>>> around that while reviewing this patch and I haven't come out the other
>>> side any happier with this approach than I was back in
>>> 20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)
>>>
>>> There are other things that don't look right to me, such as what's going
>>> on at the bottom of push_update_down(), but I don't think there's much
>>> point going into it until we figure out what the core FDW API here
>>> should look like. It might not be all that far from what we have now,
>>> but I don't think we can just ignore the existing, documented, API.

>> OK, I'll try to introduce the core FDW API for this (and make changes to the
>> core code) to address your previous comments.

> I'm a bit behind in reading up on this, so maybe it's been covered
> since, but is there a discussion of this API on another thread, or a
> newer patch available?

Actually, I'm now working on this. My basic idea is to add new FDW APIs
for handling the bulk work, in order not to make messy the prescribed
"Update/Delete one tuple" interface; 1) add to nodeModifyTable.c or
nodeForeignscan.c the new FDW APIs BeginPushedDownForeignModify,
ExecPushedDownForeignModify and EndPushedDownForeignModify for that, and
2) call these FDW APIs, instead of BeginForeignModify,
ExecForeignUpdate/ExecForeignDelete and EndForeignModify, when doing
update pushdown. I'd like to propose that in more detail as soon as
possible, probably with an updated patch.

Best regards,
Etsuro Fujita

On 2015/11/26 18:00, Etsuro Fujita wrote:
> On 2015/11/25 20:36, Thom Brown wrote:
>> On 13 May 2015 at 04:10, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
>> wrote:
>>> On 2015/05/13 0:55, Stephen Frost wrote:
>>>> While the EXPLAIN output changed, the structure hasn't really changed
>>>> from what was discussed previously and there's not been any real
>>>> involvment from the core code in what's happening here.
>>>>
>>>> Clearly, the documentation around how to use the FDW API hasn't changed
>>>> at all and there's been no additions to it for handling bulk work.
>>>> Everything here continues to be done inside of postgres_fdw, which
>>>> essentially ignores the prescribed "Update/Delete one tuple" interface
>>>> for ExecForeignUpdate/ExecForeignDelete.
>>>>
>>>> I've spent the better part of the past two days trying to reason my way
>>>> around that while reviewing this patch and I haven't come out the other
>>>> side any happier with this approach than I was back in
>>>> 20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)
>>>>
>>>> There are other things that don't look right to me, such as what's
>>>> going
>>>> on at the bottom of push_update_down(), but I don't think there's much
>>>> point going into it until we figure out what the core FDW API here
>>>> should look like. It might not be all that far from what we have now,
>>>> but I don't think we can just ignore the existing, documented, API.

>>> OK, I'll try to introduce the core FDW API for this (and make changes
>>> to the
>>> core code) to address your previous comments.

>> I'm a bit behind in reading up on this, so maybe it's been covered
>> since, but is there a discussion of this API on another thread, or a
>> newer patch available?

To address Stephen's comments, I'd like to propose the following FDW APIs:

bool
PlanDMLPushdown (PlannerInfo *root,
ModifyTable *plan,
Index resultRelation,
int subplan_index);

This is called in make_modifytable, before calling PlanForeignModify.
This checks to see whether a given UPDATE/DELETE .. RETURNING .. is
pushdown-safe and if so, performs planning actions needed for the DML
pushdown. The idea is to modify a ForeignScan subplan accordingly as in
the previous patch. If the DML is pushdown-safe, this returns true, and
we don't call PlanForeignModify anymore. (Else returns false and call
PlanForeignModify as before.) When the DML is pushdown-safe, we hook
the following FDW APIs located in nodeForeignscan.c, instead of
BeginForeignModify, ExecForeignUpdate/ExecForeignDelete and
EndForeignModify:

void
BeginDMLPushdown (ForeignScanState *node,
int eflags);

This initializes the DML pushdown, like BeginForeignScan.

TupleTableSlot *
IterateDMLPushdown (ForeignScanState *node);

This fetches one returning result from the foreign server, like
IterateForeignScan, if having a RETURNING clause. If not, just return
an empty slot. (I'm thinking that it's required that the FDW replaces
the targetlist of the ForeignScan subplan to the RETURNING clause during
PlanDMLPushdown, if having the clause, so that we do nothing at
ModifyTable.)

void
EndDMLPushdown (ForeignScanState *node);

This finishes the DML pushdown, like EndForeignScan.

I'm attaching a WIP patch, which only includes changes to the core. I'm
now working on the postgres_fdw patch to demonstrate that these APIs
work well, but I'd be happy if I could get any feedback earlier.

Best regards,
Etsuro Fujita

On Mon, Dec 21, 2015 at 6:20 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2015/11/26 18:00, Etsuro Fujita wrote:
>
>> On 2015/11/25 20:36, Thom Brown wrote:
>>
>>> On 13 May 2015 at 04:10, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
>>> wrote:
>>>
>>>> On 2015/05/13 0:55, Stephen Frost wrote:
>>>>
>>>>> While the EXPLAIN output changed, the structure hasn't really changed
>>>>> from what was discussed previously and there's not been any real
>>>>> involvment from the core code in what's happening here.
>>>>>
>>>>> Clearly, the documentation around how to use the FDW API hasn't changed
>>>>> at all and there's been no additions to it for handling bulk work.
>>>>> Everything here continues to be done inside of postgres_fdw, which
>>>>> essentially ignores the prescribed "Update/Delete one tuple" interface
>>>>> for ExecForeignUpdate/ExecForeignDelete.
>>>>>
>>>>> I've spent the better part of the past two days trying to reason my way
>>>>> around that while reviewing this patch and I haven't come out the other
>>>>> side any happier with this approach than I was back in
>>>>> 20140911153049(dot)GC16422(at)tamriel(dot)snowman(dot)net(dot)
>>>>>
>>>>> There are other things that don't look right to me, such as what's
>>>>> going
>>>>> on at the bottom of push_update_down(), but I don't think there's much
>>>>> point going into it until we figure out what the core FDW API here
>>>>> should look like. It might not be all that far from what we have now,
>>>>> but I don't think we can just ignore the existing, documented, API.
>>>>>
>>>>
> OK, I'll try to introduce the core FDW API for this (and make changes
>>>> to the
>>>> core code) to address your previous comments.
>>>>
>>>
> I'm a bit behind in reading up on this, so maybe it's been covered
>>> since, but is there a discussion of this API on another thread, or a
>>> newer patch available?
>>>
>>
> To address Stephen's comments, I'd like to propose the following FDW APIs:
>
> bool
> PlanDMLPushdown (PlannerInfo *root,
> ModifyTable *plan,
> Index resultRelation,
> int subplan_index);
>
> This is called in make_modifytable, before calling PlanForeignModify. This
> checks to see whether a given UPDATE/DELETE .. RETURNING .. is
> pushdown-safe and if so, performs planning actions needed for the DML
> pushdown. The idea is to modify a ForeignScan subplan accordingly as in
> the previous patch. If the DML is pushdown-safe, this returns true, and we
> don't call PlanForeignModify anymore. (Else returns false and call
> PlanForeignModify as before.) When the DML is pushdown-safe, we hook the
> following FDW APIs located in nodeForeignscan.c, instead of
> BeginForeignModify, ExecForeignUpdate/ExecForeignDelete and
> EndForeignModify:
>
> void
> BeginDMLPushdown (ForeignScanState *node,
> int eflags);
>
> This initializes the DML pushdown, like BeginForeignScan.
>
> TupleTableSlot *
> IterateDMLPushdown (ForeignScanState *node);
>
> This fetches one returning result from the foreign server, like
> IterateForeignScan, if having a RETURNING clause. If not, just return an
> empty slot. (I'm thinking that it's required that the FDW replaces the
> targetlist of the ForeignScan subplan to the RETURNING clause during
> PlanDMLPushdown, if having the clause, so that we do nothing at
> ModifyTable.)
>
> void
> EndDMLPushdown (ForeignScanState *node);
>
> This finishes the DML pushdown, like EndForeignScan.
>

+1.

I like idea of separate FDW API for the DML Pushdown. Was thinking can't we
can re-use the IterateForeignScan(ForeignScanState *node) rather then
introducing IterateDMLPushdown(ForeignScanState *node) new API ?

> I'm attaching a WIP patch, which only includes changes to the core. I'm
> now working on the postgres_fdw patch to demonstrate that these APIs work
> well, but I'd be happy if I could get any feedback earlier.
>
> Best regards,
> Etsuro Fujita
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Rushabh Lathia

On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia
<rushabh(dot)lathia(at)gmail(dot)com> wrote:
> +1.
>
> I like idea of separate FDW API for the DML Pushdown. Was thinking can't we
> can re-use the IterateForeignScan(ForeignScanState *node) rather then
> introducing IterateDMLPushdown(ForeignScanState *node) new API ?

Yeah, I think we need to ask ourselves what advantage we're getting
out of adding any new core APIs. Marking the scan as a pushed-down
update or delete has some benefit in terms of making the information
visible via EXPLAIN, but even that's a pretty thin benefit. The
iterate method seems to just complicate the core code without any
benefit at all. More generally, there is very, very little code in
this patch that accomplishes anything that could not be done just as
well with the existing methods. So why are we even doing these core
changes?

Tom seemed to think that we could centralize some checks in the core
code, say, related to triggers, or to LIMIT. But there's nothing like
that in this patch, so I'm not really understanding the point.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015/12/24 4:34, Robert Haas wrote:
> On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia
> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
>> +1.
>>
>> I like idea of separate FDW API for the DML Pushdown. Was thinking can't we
>> can re-use the IterateForeignScan(ForeignScanState *node) rather then
>> introducing IterateDMLPushdown(ForeignScanState *node) new API ?

> Yeah, I think we need to ask ourselves what advantage we're getting
> out of adding any new core APIs. Marking the scan as a pushed-down
> update or delete has some benefit in terms of making the information
> visible via EXPLAIN, but even that's a pretty thin benefit. The
> iterate method seems to just complicate the core code without any
> benefit at all. More generally, there is very, very little code in
> this patch that accomplishes anything that could not be done just as
> well with the existing methods. So why are we even doing these core
> changes?

From the FDWs' point of view, ISTM that what FDWs have to do for
IterateDMLPushdown is quite different from what FDWs have to do for
IterateForeignScan; eg, IterateDMLPushdown must work in accordance with
presence/absence of a RETURNING list. (In addition to that,
IterateDMLPushdown has been designed so that it must make the scan tuple
available to later RETURNING projection in nodeModifyTable.c.) So, I
think that it's better to FDWs to add separate APIs for the DML
pushdown, making the FDW code much simpler. So based on that idea, I
added the postgres_fdw changes to the patch. Attached is an updated
version of the patch, which is still WIP, but I'd be happy if I could
get any feedback.

> Tom seemed to think that we could centralize some checks in the core
> code, say, related to triggers, or to LIMIT. But there's nothing like
> that in this patch, so I'm not really understanding the point.

For the trigger check, I added relation_has_row_level_triggers. I
placed that function in postgres_fdw.c in the updated patch, but I think
that by placing that function in the core, FDWs can share that function.
As for the LIMIT, I'm not sure we can do something about that.

I think the current design allows us to handle a pushed-down update on a
join, ie, "UPDATE foo ... FROM bar ..." where both foo and bar are
remote, which was Tom's concern, but I'll leave that for another patch.
Also, I think the current design could also extend to push down INSERT
.. RETURNING .., but I'd like to leave that for future work.

I'll add this to the next CF.

Best regards,
Etsuro Fujita

I started looking at updated patch and its definitely iked the new
approach.

Patch passing the mandatory checks:

1) Patch applied cleanly using patch -p1 command
2) Source got compiled cleanly
3) make check, running cleanly

Explain output for the remote table and inheritance relations looks better
then earlier version of patch.

-- Inheritance foreign relation
postgres=# explain (ANALYZE, VERBOSE) update fp set a = 20;
QUERY
PLAN
-----------------------------------------------------------------------------------------------------------------------
Update on public.fp (cost=100.00..767.60 rows=10920 width=6) (actual
time=1.000..1.000 rows=0 loops=1)
Foreign Update on public.fp
Foreign Update on public.fc1
Foreign Update on public.fc2
Foreign Update on public.fc3
-> Foreign Update on public.fp (cost=100.00..191.90 rows=2730 width=6)
(actual time=0.493..0.493 rows=0 loops=1)
Remote SQL: UPDATE public.p SET a = 20
-> Foreign Update on public.fc1 (cost=100.00..191.90 rows=2730
width=6) (actual time=0.177..0.177 rows=0 loops=1)
Remote SQL: UPDATE public.c1 SET a = 20
-> Foreign Update on public.fc2 (cost=100.00..191.90 rows=2730
width=6) (actual time=0.163..0.163 rows=0 loops=1)
Remote SQL: UPDATE public.c2 SET a = 20
-> Foreign Update on public.fc3 (cost=100.00..191.90 rows=2730
width=6) (actual time=0.158..0.158 rows=0 loops=1)
Remote SQL: UPDATE public.c3 SET a = 20
Planning time: 0.228 ms
Execution time: 1.359 ms
(15 rows)

-- Foreign table update
postgres=# explain (ANALYZE, VERBOSE) update ft1 set c8 = 'updated' where
c1 = '200';
QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..116.13 rows=2 width=144) (actual
time=0.485..0.485 rows=0 loops=1)
-> Foreign Update on public.ft1 (cost=100.00..116.13 rows=2 width=144)
(actual time=0.483..0.483 rows=0 loops=1)
Remote SQL: UPDATE public.t1 SET c8 = 'updated '::character(10)
WHERE ((c1 = 200))
Planning time: 0.158 ms
Execution time: 0.786 ms
(5 rows)

-- Explain output for the returning clause:
postgres=# explain (ANALYZE, VERBOSE) update ft1 set c8 = 'updated' where
c1 = '200' returning c2;
QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..116.13 rows=2 width=144) (actual
time=0.516..0.516 rows=0 loops=1)
Output: c2
-> Foreign Update on public.ft1 (cost=100.00..116.13 rows=2 width=144)
(actual time=0.514..0.514 rows=0 loops=1)
Remote SQL: UPDATE public.t1 SET c8 = 'updated '::character(10)
WHERE ((c1 = 200)) RETURNING c2
Planning time: 0.172 ms
Execution time: 0.938 ms
(6 rows)

-- Explain output when returning clause is not pushdown safe:
postgres=# explain (ANALYZE, VERBOSE) update ft1 set c8 = 'updated' where
c1 = '200' returning local_func(20);
QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..116.13 rows=2 width=144) (actual
time=0.364..0.364 rows=0 loops=1)
Output: local_func(20)
-> Foreign Update on public.ft1 (cost=100.00..116.13 rows=2 width=144)
(actual time=0.363..0.363 rows=0 loops=1)
Remote SQL: UPDATE public.t1 SET c8 = 'updated '::character(10)
WHERE ((c1 = 200))
Planning time: 0.142 ms
Execution time: 0.623 ms
(6 rows)

-- Explain output with PREPARE:
postgres=# explain (ANALYZE, VERBOSE) execute ftupdate(20);
QUERY
PLAN
----------------------------------------------------------------------------------------------------------------------
Update on public.ft1 (cost=100.00..116.13 rows=2 width=144) (actual
time=0.712..0.712 rows=0 loops=1)
-> Foreign Update on public.ft1 (cost=100.00..116.13 rows=2 width=144)
(actual time=0.711..0.711 rows=1 loops=1)
Remote SQL: UPDATE public.t1 SET c8 = 'updated '::character(10)
WHERE ((c1 = 20))
Execution time: 1.001 ms
(4 rows)

With the initial look and test overall things looking great, I am still
reviewing the code changes but here are few early doubts/questions:

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

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

.) Documentation for the new API is missing (fdw-callbacks).

Regards,

On Fri, Dec 25, 2015 at 3:30 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2015/12/24 4:34, Robert Haas wrote:
>
>> On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia
>> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
>>
>>> +1.
>>>
>>> I like idea of separate FDW API for the DML Pushdown. Was thinking can't
>>> we
>>> can re-use the IterateForeignScan(ForeignScanState *node) rather then
>>> introducing IterateDMLPushdown(ForeignScanState *node) new API ?
>>>
>>
> Yeah, I think we need to ask ourselves what advantage we're getting
>> out of adding any new core APIs. Marking the scan as a pushed-down
>> update or delete has some benefit in terms of making the information
>> visible via EXPLAIN, but even that's a pretty thin benefit. The
>> iterate method seems to just complicate the core code without any
>> benefit at all. More generally, there is very, very little code in
>> this patch that accomplishes anything that could not be done just as
>> well with the existing methods. So why are we even doing these core
>> changes?
>>
>
> From the FDWs' point of view, ISTM that what FDWs have to do for
> IterateDMLPushdown is quite different from what FDWs have to do for
> IterateForeignScan; eg, IterateDMLPushdown must work in accordance with
> presence/absence of a RETURNING list. (In addition to that,
> IterateDMLPushdown has been designed so that it must make the scan tuple
> available to later RETURNING projection in nodeModifyTable.c.) So, I think
> that it's better to FDWs to add separate APIs for the DML pushdown, making
> the FDW code much simpler. So based on that idea, I added the postgres_fdw
> changes to the patch. Attached is an updated version of the patch, which
> is still WIP, but I'd be happy if I could get any feedback.
>
> Tom seemed to think that we could centralize some checks in the core
>> code, say, related to triggers, or to LIMIT. But there's nothing like
>> that in this patch, so I'm not really understanding the point.
>>
>
> For the trigger check, I added relation_has_row_level_triggers. I placed
> that function in postgres_fdw.c in the updated patch, but I think that by
> placing that function in the core, FDWs can share that function. As for
> the LIMIT, I'm not sure we can do something about that.
>
> I think the current design allows us to handle a pushed-down update on a
> join, ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote,
> which was Tom's concern, but I'll leave that for another patch. Also, I
> think the current design could also extend to push down INSERT .. RETURNING
> .., but I'd like to leave that for future work.
>
> I'll add this to the next CF.
>
> Best regards,
> Etsuro Fujita
>

Rushabh Lathia

On 25 December 2015 at 10:00, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2015/12/24 4:34, Robert Haas wrote:
>>
>> On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia
>> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
>>>
>>> +1.
>>>
>>> I like idea of separate FDW API for the DML Pushdown. Was thinking can't
>>> we
>>> can re-use the IterateForeignScan(ForeignScanState *node) rather then
>>> introducing IterateDMLPushdown(ForeignScanState *node) new API ?
>
>
>> Yeah, I think we need to ask ourselves what advantage we're getting
>> out of adding any new core APIs. Marking the scan as a pushed-down
>> update or delete has some benefit in terms of making the information
>> visible via EXPLAIN, but even that's a pretty thin benefit. The
>> iterate method seems to just complicate the core code without any
>> benefit at all. More generally, there is very, very little code in
>> this patch that accomplishes anything that could not be done just as
>> well with the existing methods. So why are we even doing these core
>> changes?
>
>
> From the FDWs' point of view, ISTM that what FDWs have to do for
> IterateDMLPushdown is quite different from what FDWs have to do for
> IterateForeignScan; eg, IterateDMLPushdown must work in accordance with
> presence/absence of a RETURNING list. (In addition to that,
> IterateDMLPushdown has been designed so that it must make the scan tuple
> available to later RETURNING projection in nodeModifyTable.c.) So, I think
> that it's better to FDWs to add separate APIs for the DML pushdown, making
> the FDW code much simpler. So based on that idea, I added the postgres_fdw
> changes to the patch. Attached is an updated version of the patch, which is
> still WIP, but I'd be happy if I could get any feedback.
>
>> Tom seemed to think that we could centralize some checks in the core
>> code, say, related to triggers, or to LIMIT. But there's nothing like
>> that in this patch, so I'm not really understanding the point.
>
>
> For the trigger check, I added relation_has_row_level_triggers. I placed
> that function in postgres_fdw.c in the updated patch, but I think that by
> placing that function in the core, FDWs can share that function. As for the
> LIMIT, I'm not sure we can do something about that.
>
> I think the current design allows us to handle a pushed-down update on a
> join, ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote,
> which was Tom's concern, but I'll leave that for another patch. Also, I
> think the current design could also extend to push down INSERT .. RETURNING
> .., but I'd like to leave that for future work.
>
> I'll add this to the next CF.

I've run into an issue:

*# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
tableoid::regclass;
ERROR:
CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
WHERE ((id = 16)) RETURNING NULL

However, this works:

*# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
tableoid::regclass, *;
tableoid | id | name | company | registered_date |
expiry_date | active | status | account_level
-----------------+----+-------+---------------+-----------------+-------------+--------+---------+---------------
local_customers | 22 | Bruce | Jo's Cupcakes | 2015-01-15 |
2017-01-14 | t | running | basic
(1 row)

In this example, "local_customers" inherits from the remote table
"public"."customers", which inherits again from the local table
"master_customers"

Same issue with DELETE of course, and the ::regclass isn't important here.

Thom

On 2016/01/06 18:58, Rushabh Lathia wrote:
> I started looking at updated patch and its definitely iked the new
> approach.

Thanks for the review!

> With the initial look and test overall things looking great, I am still
> reviewing the code changes but here are few early doubts/questions:

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

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

I think this depends on the capabilities of the FDW.

> .) Documentation for the new API is missing (fdw-callbacks).

Will add the docs.

Best regards,
Etsuro Fujita

On 2016/01/06 20:37, Thom Brown wrote:
> On 25 December 2015 at 10:00, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Attached is an updated version of the patch, which is
>> still WIP, but I'd be happy if I could get any feedback.

> I've run into an issue:
>
> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
> tableoid::regclass;
> ERROR:
> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
> WHERE ((id = 16)) RETURNING NULL
>
> However, this works:
>
> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
> tableoid::regclass, *;
> tableoid | id | name | company | registered_date |
> expiry_date | active | status | account_level
> -----------------+----+-------+---------------+-----------------+-------------+--------+---------+---------------
> local_customers | 22 | Bruce | Jo's Cupcakes | 2015-01-15 |
> 2017-01-14 | t | running | basic
> (1 row)
>
> In this example, "local_customers" inherits from the remote table
> "public"."customers", which inherits again from the local table
> "master_customers"
>
> Same issue with DELETE of course, and the ::regclass isn't important here.

Will fix.

Thanks for the testing!

Best regards,
Etsuro Fujita

On 2016/01/07 21:50, Etsuro Fujita wrote:
> On 2016/01/06 20:37, Thom Brown wrote:
>> On 25 December 2015 at 10:00, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> Attached is an updated version of the patch, which is
>>> still WIP, but I'd be happy if I could get any feedback.

>> I've run into an issue:
>>
>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>> tableoid::regclass;
>> ERROR:
>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>> WHERE ((id = 16)) RETURNING NULL

> Will fix.

While working on this, I noticed that the existing postgres_fdw system
shows similar behavior, so I changed the subject.

IIUC, the reason for that is when the local query specifies "RETURNING
tableoid::regclass", the FDW has fmstate->has_returning=false while the
remote query executed at ModifyTable has "RETURNING NULL", as shown in
the above example; that would cause an abnormal exit in executing the
remote query in postgresExecForeignUpdate, since that the FDW would get
PGRES_TUPLES_OK as a result of the query while the FDW would think that
the right result to get should be PGRES_COMMAND_OK, from the flag
fmstate->has_returning=false.

Attached is a patch to fix that.

Best regards,
Etsuro Fujita

On 2016/01/07 21:45, Etsuro Fujita wrote:
> On 2016/01/06 18:58, Rushabh Lathia wrote:
>> .) Documentation for the new API is missing (fdw-callbacks).

> Will add the docs.

I added docs for new FDW APIs.

Other changes:

* Rename relation_has_row_level_triggers to relation_has_row_triggers
shortly, and move it to rewriteHandler.c. I'm not sure rewriteHandler.c
is a good place for that, though.

* Revise code, including a helper function get_result_result, whcih I
implemented using a modified version of store_returning_result in the
previous patch, but on second thought, I think that that is a bit too
invasive. So, I re-implemented that function directly using
make_tuple_from_result_row.

* Add more comments.

* Add more regression tests.

Attached is an updated version of the patch. Comments are wellcome!
(If the fix [1] is okay, I'd like to update this patch on top of the
patch in [1].)

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/568F4430.6060805@lab.ntt.co.jp

On Thu, Jan 7, 2016 at 6:15 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2016/01/06 18:58, Rushabh Lathia wrote:
>
>> I started looking at updated patch and its definitely iked the new
>> approach.
>>
>
> Thanks for the review!
>
> With the initial look and test overall things looking great, I am still
>> reviewing the code changes but here are few early doubts/questions:
>>
>
> .) 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 ?

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

I think this depends on the capabilities of the FDW.
>
> .) Documentation for the new API is missing (fdw-callbacks).
>>
>
> Will add the docs.
>

--
Rushabh Lathia

On 8 January 2016 at 05:08, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2016/01/07 21:50, Etsuro Fujita wrote:
>>
>> On 2016/01/06 20:37, Thom Brown wrote:
>>>
>>> On 25 December 2015 at 10:00, Etsuro Fujita
>>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>
>>>> Attached is an updated version of the patch, which is
>>>> still WIP, but I'd be happy if I could get any feedback.
>
>
>>> I've run into an issue:
>>>
>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>> tableoid::regclass;
>>> ERROR:
>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>> WHERE ((id = 16)) RETURNING NULL
>
>
>> Will fix.
>
>
> While working on this, I noticed that the existing postgres_fdw system shows
> similar behavior, so I changed the subject.
>
> IIUC, the reason for that is when the local query specifies "RETURNING
> tableoid::regclass", the FDW has fmstate->has_returning=false while the
> remote query executed at ModifyTable has "RETURNING NULL", as shown in the
> above example; that would cause an abnormal exit in executing the remote
> query in postgresExecForeignUpdate, since that the FDW would get
> PGRES_TUPLES_OK as a result of the query while the FDW would think that the
> right result to get should be PGRES_COMMAND_OK, from the flag
> fmstate->has_returning=false.
>
> Attached is a patch to fix that.

I can't apply this patch in tandem with FDW DML pushdown patch (either
v2 or v3).

Thom

On 2016/01/12 20:36, Thom Brown wrote:
> On 8 January 2016 at 05:08, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

>>> On 2016/01/06 20:37, Thom Brown wrote:
>>>> I've run into an issue:
>>>>
>>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>>> tableoid::regclass;
>>>> ERROR:
>>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>>> WHERE ((id = 16)) RETURNING NULL

>> While working on this, I noticed that the existing postgres_fdw system shows
>> similar behavior, so I changed the subject.
>>
>> IIUC, the reason for that is when the local query specifies "RETURNING
>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>> remote query executed at ModifyTable has "RETURNING NULL", as shown in the
>> above example; that would cause an abnormal exit in executing the remote
>> query in postgresExecForeignUpdate, since that the FDW would get
>> PGRES_TUPLES_OK as a result of the query while the FDW would think that the
>> right result to get should be PGRES_COMMAND_OK, from the flag
>> fmstate->has_returning=false.

>> Attached is a patch to fix that.

> I can't apply this patch in tandem with FDW DML pushdown patch (either
> v2 or v3).

That patch is for fixing the similar issue in the existing postgres_fdw
system. So, please apply that patch without the DML pushdown patch. If
that patch is reasonable as a fix for the issue, I'll update the DML
pushdown patch (v3) on top of that patch.

Best regards,
Etsuro Fujita

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

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

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

On 2016/01/08 14:08, Etsuro Fujita wrote:
> On 2016/01/07 21:50, Etsuro Fujita wrote:
>> On 2016/01/06 20:37, Thom Brown wrote:

>>> I've run into an issue:
>>>
>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>> tableoid::regclass;
>>> ERROR:
>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>> WHERE ((id = 16)) RETURNING NULL

> While working on this, I noticed that the existing postgres_fdw system
> shows similar behavior, so I changed the subject.
>
> IIUC, the reason for that is when the local query specifies "RETURNING
> tableoid::regclass", the FDW has fmstate->has_returning=false while the
> remote query executed at ModifyTable has "RETURNING NULL", as shown in
> the above example; that would cause an abnormal exit in executing the
> remote query in postgresExecForeignUpdate, since that the FDW would get
> PGRES_TUPLES_OK as a result of the query while the FDW would think that
> the right result to get should be PGRES_COMMAND_OK, from the flag
> fmstate->has_returning=false.
>
> Attached is a patch to fix that.

I added this to the next CF.

https://commitfest.postgresql.org/9/483/

Best regards,
Etsuro Fujita

On 12 January 2016 at 11:49, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2016/01/12 20:36, Thom Brown wrote:
>>
>> On 8 January 2016 at 05:08, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
>> wrote:
>
>
>>>> On 2016/01/06 20:37, Thom Brown wrote:
>>>>>
>>>>> I've run into an issue:
>>>>>
>>>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>>>> tableoid::regclass;
>>>>> ERROR:
>>>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>>>> WHERE ((id = 16)) RETURNING NULL
>
>
>>> While working on this, I noticed that the existing postgres_fdw system
>>> shows
>>> similar behavior, so I changed the subject.
>>>
>>> IIUC, the reason for that is when the local query specifies "RETURNING
>>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>>> the
>>> above example; that would cause an abnormal exit in executing the remote
>>> query in postgresExecForeignUpdate, since that the FDW would get
>>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>>> the
>>> right result to get should be PGRES_COMMAND_OK, from the flag
>>> fmstate->has_returning=false.
>
>
>>> Attached is a patch to fix that.
>
>
>> I can't apply this patch in tandem with FDW DML pushdown patch (either
>> v2 or v3).
>
>
> That patch is for fixing the similar issue in the existing postgres_fdw
> system. So, please apply that patch without the DML pushdown patch. If
> that patch is reasonable as a fix for the issue, I'll update the DML
> pushdown patch (v3) on top of that patch.

The patch seems to work for me:

Before:

*# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
tableoid::regclass;
ERROR:
CONTEXT: Remote SQL command: UPDATE public.customers SET id = $2
WHERE ctid = $1 RETURNING NULL

After:

*# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
tableoid::regclass;
tableoid
------------------
remote.customers
(1 row)

UPDATE 1

Thom

On Tue, Jan 19, 2016 at 1:59 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>> I've run into an issue:
>>>>
>>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>>> tableoid::regclass;
>>>> ERROR:
>>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>>> WHERE ((id = 16)) RETURNING NULL
>
>> While working on this, I noticed that the existing postgres_fdw system
>> shows similar behavior, so I changed the subject.
>>
>> IIUC, the reason for that is when the local query specifies "RETURNING
>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>> the above example; that would cause an abnormal exit in executing the
>> remote query in postgresExecForeignUpdate, since that the FDW would get
>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>> the right result to get should be PGRES_COMMAND_OK, from the flag
>> fmstate->has_returning=false.
>>
>> Attached is a patch to fix that.
>
> I added this to the next CF.
>
> https://commitfest.postgresql.org/9/483/

Uggh, what a mess. How about passing an additional boolean
"is_returning" to deparseTargetList()? If false, then
deparseTargetList() behaves as now. If false, then
deparseTargetList() doesn't append anything at all if there are no
columns to return, instead of (as at present) adding NULL. On the
other hand, if there are columns to return, then it appends "
RETURNING " before the first column. Then, deparseReturningList could
skip adding RETURNING itself, and just pass true to
deparseTargetList(). The advantage of this approach is that we don't
end up with two copies of the code that have to stay synchronized -
the decision is made inside deparseTargetList(), and
deparseReturningList() accepts the results.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016/01/20 3:42, Robert Haas wrote:
> On Tue, Jan 19, 2016 at 1:59 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>> I've run into an issue:
>>>>>
>>>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>>>> tableoid::regclass;
>>>>> ERROR:
>>>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>>>> WHERE ((id = 16)) RETURNING NULL

>>> While working on this, I noticed that the existing postgres_fdw system
>>> shows similar behavior, so I changed the subject.
>>>
>>> IIUC, the reason for that is when the local query specifies "RETURNING
>>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>>> the above example; that would cause an abnormal exit in executing the
>>> remote query in postgresExecForeignUpdate, since that the FDW would get
>>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>>> the right result to get should be PGRES_COMMAND_OK, from the flag
>>> fmstate->has_returning=false.
>>>
>>> Attached is a patch to fix that.

>> I added this to the next CF.
>>
>> https://commitfest.postgresql.org/9/483/

> Uggh, what a mess. How about passing an additional boolean
> "is_returning" to deparseTargetList()? If false, then
> deparseTargetList() behaves as now. If false, then
> deparseTargetList() doesn't append anything at all if there are no
> columns to return, instead of (as at present) adding NULL. On the
> other hand, if there are columns to return, then it appends "
> RETURNING " before the first column. Then, deparseReturningList could
> skip adding RETURNING itself, and just pass true to
> deparseTargetList(). The advantage of this approach is that we don't
> end up with two copies of the code that have to stay synchronized -

Thanks for the review! I think that is important.

> the decision is made inside deparseTargetList(), and
> deparseReturningList() accepts the results.

My concern about that is that would make the code in deparseTargetList()
complicated.

Essentially, I think your propossal needs a two-pass algorithm for
deparseTargetList; (1) create an integer List of the columns being
retrieved from the given attrs_used (getRetrievedAttrs()), and (2) print
those columns (printRetrievedAttrs()). How about sharing those two
functions between deparseTargetList and deparseReturningList?:

* In deparseTargetList, perform getRetrievedAttrs(). If
getRetrievedAttrs()!=NIL, perform printRetrievedAttrs(). Otherwise,
print NULL.
* In deparseReturningList, perform getRetrievedAttrs() before adding
RETURNING. If getRetrievedAttrs()!=NIL, print RETURNING and perform
printRetrievedAttrs(). Otherwise, do nothing.

Best regards,
Etsuro Fujita

On Fri, Jan 15, 2016 at 9:06 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> 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)
>
>
Oops sorry. I got the point now.

> 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.
>
>
Overall I am quite done with the review of this patch. Patch is in good
shape and covered most of the things which been discussed earlier
or been mentioned during review process. Patch pass through the
make check and also includes good test coverage.

Here are couple of things which is still open for discussion:

1)

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

This need to handle by the Owner.

2) Decision on whether we need the separate new node ForeignUpdate,
ForeignDelete. In my opinion I really don't see the need of this as we
that will add lot of duplicate. Having said that if committer or someone
else feel like that will make code more clean that is also true,

This need more comments from the committer.

Thanks

Rushabh Lathia
www.EnterpriseDB.com

On Wed, Jan 20, 2016 at 4:50 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> My concern about that is that would make the code in deparseTargetList()
> complicated.
>
> Essentially, I think your propossal needs a two-pass algorithm for
> deparseTargetList; (1) create an integer List of the columns being retrieved
> from the given attrs_used (getRetrievedAttrs()), and (2) print those columns
> (printRetrievedAttrs()). How about sharing those two functions between
> deparseTargetList and deparseReturningList?:

I don't see why we'd need that. I adjusted the code in postgres_fdw
along the lines I had in mind and am attaching the result. It doesn't
look complicated to me, and it passes the regression test you wrote.

By the way, I'm not too sure I understand the need for the core
changes that are part of this patch, and I think that point merits
some discussion. Whenever you change core like this, you're changing
the contract between the FDW and core; it's not just postgres_fdw that
needs updating, but every FDW. So we'd better be pretty sure we need
these changes and they are adequately justified before we think about
putting them into the tree. Are these core changes really needed
here, or can we fix this whole issue in postgres_fdw and leave the
core code alone?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016/01/21 5:06, Robert Haas wrote:
> On Wed, Jan 20, 2016 at 4:50 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> My concern about that is that would make the code in deparseTargetList()
>> complicated.
>>
>> Essentially, I think your propossal needs a two-pass algorithm for
>> deparseTargetList; (1) create an integer List of the columns being retrieved
>> from the given attrs_used (getRetrievedAttrs()), and (2) print those columns
>> (printRetrievedAttrs()). How about sharing those two functions between
>> deparseTargetList and deparseReturningList?:

> I don't see why we'd need that. I adjusted the code in postgres_fdw
> along the lines I had in mind and am attaching the result. It doesn't
> look complicated to me, and it passes the regression test you wrote.

Thanks for the patch! From the patch, I correctly understand what you
proposed. Good idea!

> By the way, I'm not too sure I understand the need for the core
> changes that are part of this patch, and I think that point merits
> some discussion. Whenever you change core like this, you're changing
> the contract between the FDW and core; it's not just postgres_fdw that
> needs updating, but every FDW. So we'd better be pretty sure we need
> these changes and they are adequately justified before we think about
> putting them into the tree. Are these core changes really needed
> here, or can we fix this whole issue in postgres_fdw and leave the
> core code alone?

Well, if we think it is the FDW's responsibility to insert a valid value
for tableoid in the returned slot during ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete, we don't need those core
changes. However, I think it would be better that that's done by
ModifyTable in the same way as ForeignScan does in ForeignNext, IMO.
That eliminates the need for postgres_fdw or any other FDW to do that
business in the callback routines.

Best regards,
Etsuro Fujita

On 2016/01/20 19:57, Rushabh Lathia wrote:
> Overall I am quite done with the review of this patch. Patch is in good
> shape and covered most of the things which been discussed earlier
> or been mentioned during review process. Patch pass through the
> make check and also includes good test coverage.

Thanks for the review!

> Here are couple of things which is still open for discussion:

> 1)
> .) 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.
>
> This need to handle by the Owner.

Done. For that, I modified relation_has_row_triggers a bit, renamed it
to has_row_triggers (more shortly), and moved it to plancat.c. And I
merged dml_is_pushdown_safe with postgresPlanDMLPushdown, and revised
that callback routine a bit. Attached is an updated version of the
patch created on top of Robert's version of the patch [1], which fixes
handling of RETURNING tableoid in updating foreign tables.

> 2) Decision on whether we need the separate new node ForeignUpdate,
> ForeignDelete. In my opinion I really don't see the need of this as we
> that will add lot of duplicate. Having said that if committer or someone
> else feel like that will make code more clean that is also true,
>
> This need more comments from the committer.

I agree with you.

Other changes:

* In previous version, I assumed that PlanDMLPushdown sets fsSystemCol
to true when rewriting the ForeignScan plan node so as to push down an
UPDATE/DELETE to the remote server, in order to initialize t_tableOid
for the scan tuple in ForeignNext. The reason is that I created the
patch so that the scan tuple is provided to the local query's RETURNING
computation, which might see the tableoid column. In this version,
however, I modified the patch so that the tableoid value is inserted by
ModifyTable. This eliminates the need for postgres_fdw (or any other
FDW) to set fsSystemCol to true in PlanDMLPushdown.

* Add set_transmission_modes/reset_transmission_modes to
deparsePushedDownUpdateSql.

* Revise comments a bit further.

* Revise docs, including a fix for a wrong copy-and-paste.

Best regards,
Etsuro Fujita

[1]
http://www.postgresql.org/message-id/CA+TgmoZ40j2uC5aC1NXu03oj4CrVOLkS15XX+PTFP-1U-8zR1Q@mail.gmail.com

On 2016/01/19 19:04, Thom Brown wrote:
> On 12 January 2016 at 11:49, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2016/01/12 20:36, Thom Brown wrote:

>>> On 8 January 2016 at 05:08, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
>>> wrote:

>>>>> On 2016/01/06 20:37, Thom Brown wrote:
>>>>>>
>>>>>> I've run into an issue:
>>>>>>
>>>>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>>>>> tableoid::regclass;
>>>>>> ERROR:
>>>>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>>>>> WHERE ((id = 16)) RETURNING NULL

>>>> While working on this, I noticed that the existing postgres_fdw system
>>>> shows
>>>> similar behavior, so I changed the subject.
>>>>
>>>> IIUC, the reason for that is when the local query specifies "RETURNING
>>>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>>>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>>>> the
>>>> above example; that would cause an abnormal exit in executing the remote
>>>> query in postgresExecForeignUpdate, since that the FDW would get
>>>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>>>> the
>>>> right result to get should be PGRES_COMMAND_OK, from the flag
>>>> fmstate->has_returning=false.

>>>> Attached is a patch to fix that.

>>> I can't apply this patch in tandem with FDW DML pushdown patch (either
>>> v2 or v3).

>> That patch is for fixing the similar issue in the existing postgres_fdw
>> system. So, please apply that patch without the DML pushdown patch. If
>> that patch is reasonable as a fix for the issue, I'll update the DML
>> pushdown patch (v3) on top of that patch.

> The patch seems to work for me:
>
> Before:
>
> *# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
> tableoid::regclass;
> ERROR:
> CONTEXT: Remote SQL command: UPDATE public.customers SET id = $2
> WHERE ctid = $1 RETURNING NULL
>
> After:
>
> *# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
> tableoid::regclass;
> tableoid
> ------------------
> remote.customers
> (1 row)
>
> UPDATE 1

Thanks for the testing!

I updated the DML pushdown patch on top of Robert's version of this
bugfix patch. Please see

http://www.postgresql.org/message-id/56A0A9F0.9090304@lab.ntt.co.jp

Best regards,
Etsuro Fujita

On Thu, Jan 21, 2016 at 3:20 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2016/01/20 19:57, Rushabh Lathia wrote:
>
>> Overall I am quite done with the review of this patch. Patch is in good
>> shape and covered most of the things which been discussed earlier
>> or been mentioned during review process. Patch pass through the
>> make check and also includes good test coverage.
>>
>
> Thanks for the review!
>
> Here are couple of things which is still open for discussion:
>>
>
> 1)
>> .) 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.
>>
>> This need to handle by the Owner.
>>
>
> Done. For that, I modified relation_has_row_triggers a bit, renamed it to
> has_row_triggers (more shortly), and moved it to plancat.c. And I merged
> dml_is_pushdown_safe with postgresPlanDMLPushdown, and revised that
> callback routine a bit. Attached is an updated version of the patch
> created on top of Robert's version of the patch [1], which fixes handling
> of RETURNING tableoid in updating foreign tables.
>

This looks great.

>
> 2) Decision on whether we need the separate new node ForeignUpdate,
>> ForeignDelete. In my opinion I really don't see the need of this as we
>> that will add lot of duplicate. Having said that if committer or someone
>> else feel like that will make code more clean that is also true,
>>
>> This need more comments from the committer.
>>
>
> I agree with you.
>
> Other changes:
>
> * In previous version, I assumed that PlanDMLPushdown sets fsSystemCol to
> true when rewriting the ForeignScan plan node so as to push down an
> UPDATE/DELETE to the remote server, in order to initialize t_tableOid for
> the scan tuple in ForeignNext. The reason is that I created the patch so
> that the scan tuple is provided to the local query's RETURNING computation,
> which might see the tableoid column. In this version, however, I modified
> the patch so that the tableoid value is inserted by ModifyTable. This
> eliminates the need for postgres_fdw (or any other FDW) to set fsSystemCol
> to true in PlanDMLPushdown.
>
> * Add set_transmission_modes/reset_transmission_modes to
> deparsePushedDownUpdateSql.
>
> * Revise comments a bit further.
>
> * Revise docs, including a fix for a wrong copy-and-paste.
>
> Best regards,
> Etsuro Fujita
>
> [1]
> http://www.postgresql.org/message-id/CA+TgmoZ40j2uC5aC1NXu03oj4CrVOLkS15XX+PTFP-1U-8zR1Q@mail.gmail.com
>

Here are couple of comments:

1)

int
IsForeignRelUpdatable (Relation rel);

Documentation for IsForeignUpdatable() need to change as it says:

If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
assumed
to be insertable, updatable, or deletable if the FDW provides
ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete respectively.

With introduce of DMLPushdown API now this is no more correct, as even if
FDW don't provide ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete
API
still foreign tables are assumed to be updatable or deletable with
DMLPushdown
API's, right ?

2)

+ /* SQL statement to execute remotely (as a String node) */
+ FdwDmlPushdownPrivateUpdateSql,

FdwDmlPushdownPrivateUpdateSql holds the UPDATE/DELETE query, so name should
be something like FdwDmlPushdownPrivateQuery os FdwDmlPushdownPrivateSql ?

Later I realized that for FdwModifyPrivateIndex too the index name is
FdwModifyPrivateUpdateSql even though its holding any DML query. Not sure
whether we should consider to change this or not ?

Apart from this perform sanity testing on the new patch and things working
as expected.

--
Rushabh Lathia
www.EnterpriseDB.com

On 2016/01/25 17:03, Rushabh Lathia wrote:
> Here are couple of comments:

> 1)
>
> int
> IsForeignRelUpdatable (Relation rel);

> Documentation for IsForeignUpdatable() need to change as it says:
>
> If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
> assumed
> to be insertable, updatable, or deletable if the FDW provides
> ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete respectively.
>
> With introduce of DMLPushdown API now this is no more correct, as even if
> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete API
> still foreign tables are assumed to be updatable or deletable with
> DMLPushdown
> API's, right ?

That's what I'd like to discuss.

I intentionally leave that as-is, because I think we should determine
the updatability of a foreign table in the current manner. As you
pointed out, even if the FDW doesn't provide eg, ExecForeignUpdate, an
UPDATE on a foreign table could be done using the DML pushdown APIs if
the UPDATE is *pushdown-safe*. However, since all UPDATEs on the
foreign table are not necessarily pushdown-safe, I'm not sure it's a
good idea to assume the table-level updatability if the FDW provides the
DML pushdown callback routines. To keep the existing updatability
decision, I think the FDW should provide the DML pushdown callback
routines together with ExecForeignInsert, ExecForeignUpdate, or
ExecForeignDelete. What do you think about that?

> 2)
>
> + /* SQL statement to execute remotely (as a String node) */
> + FdwDmlPushdownPrivateUpdateSql,
>
> FdwDmlPushdownPrivateUpdateSql holds the UPDATE/DELETE query, so name should
> be something like FdwDmlPushdownPrivateQuery os FdwDmlPushdownPrivateSql ?

> Later I realized that for FdwModifyPrivateIndex too the index name is
> FdwModifyPrivateUpdateSql even though its holding any DML query. Not sure
> whether we should consider to change this or not ?

To tell the truth, I imitated FdwModifyPrivateIndex when adding
FdwDmlPushdownPrivateIndex, because I think "UpdateSql" means INSERT,
UPDATE, or DELETE, not just UPDATE. (IsForeignRelUpdatable discussed
above reports not only the updatability but the insertability and
deletability of a foreign table!). So, +1 for leaving that as-is.

> Apart from this perform sanity testing on the new patch and things working
> as expected.

Thanks for the review!

Best regards,
Etsuro Fujita

On Tue, Jan 26, 2016 at 4:15 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2016/01/25 17:03, Rushabh Lathia wrote:
>
>> Here are couple of comments:
>>
>
> 1)
>>
>> int
>> IsForeignRelUpdatable (Relation rel);
>>
>
> Documentation for IsForeignUpdatable() need to change as it says:
>>
>> If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
>> assumed
>> to be insertable, updatable, or deletable if the FDW provides
>> ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete respectively.
>>
>> With introduce of DMLPushdown API now this is no more correct, as even if
>> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
>> ExecForeignDelete API
>> still foreign tables are assumed to be updatable or deletable with
>> DMLPushdown
>> API's, right ?
>>
>
> That's what I'd like to discuss.
>
> I intentionally leave that as-is, because I think we should determine the
> updatability of a foreign table in the current manner. As you pointed out,
> even if the FDW doesn't provide eg, ExecForeignUpdate, an UPDATE on a
> foreign table could be done using the DML pushdown APIs if the UPDATE is
> *pushdown-safe*. However, since all UPDATEs on the foreign table are not
> necessarily pushdown-safe, I'm not sure it's a good idea to assume the
> table-level updatability if the FDW provides the DML pushdown callback
> routines. To keep the existing updatability decision, I think the FDW
> should provide the DML pushdown callback routines together with
> ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete. What do you
> think about that?
>
>
Sorry but I am not in favour of adding compulsion that FDW should provide
the DML pushdown callback routines together with existing ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete APIs.

May be we should change the documentation in such way, that explains

a) If FDW PlanDMLPushdown is NULL, then check for ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete APIs
b) If FDW PlanDMLPushdown is non-NULL and plan is not pushable
check for ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete APIs
c) If FDW PlanDMLPushdown is non-NULL and plan is pushable
check for DMLPushdown APIs.

Does this sounds wired ?

> 2)
>>
>> + /* SQL statement to execute remotely (as a String node) */
>> + FdwDmlPushdownPrivateUpdateSql,
>>
>> FdwDmlPushdownPrivateUpdateSql holds the UPDATE/DELETE query, so name
>> should
>> be something like FdwDmlPushdownPrivateQuery os FdwDmlPushdownPrivateSql ?
>>
>
> Later I realized that for FdwModifyPrivateIndex too the index name is
>> FdwModifyPrivateUpdateSql even though its holding any DML query. Not sure
>> whether we should consider to change this or not ?
>>
>
> To tell the truth, I imitated FdwModifyPrivateIndex when adding
> FdwDmlPushdownPrivateIndex, because I think "UpdateSql" means INSERT,
> UPDATE, or DELETE, not just UPDATE. (IsForeignRelUpdatable discussed above
> reports not only the updatability but the insertability and deletability of
> a foreign table!). So, +1 for leaving that as-is.
>
>
Make sense for now.

> Apart from this perform sanity testing on the new patch and things working
>> as expected.
>>
>
> Thanks for the review!
>
> Best regards,
> Etsuro Fujita
>
>
>

--
Rushabh Lathia

On 2016/01/26 22:57, Rushabh Lathia wrote:
> On Tue, Jan 26, 2016 at 4: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/25 17:03, Rushabh Lathia wrote:

> int
> IsForeignRelUpdatable (Relation rel);

> Documentation for IsForeignUpdatable() need to change as it says:
>
> If the IsForeignRelUpdatable pointer is set to NULL, foreign
> tables are
> assumed
> to be insertable, updatable, or deletable if the FDW provides
> ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete respectively.
>
> With introduce of DMLPushdown API now this is no more correct,
> as even if
> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete API
> still foreign tables are assumed to be updatable or deletable with
> DMLPushdown
> API's, right ?

> That's what I'd like to discuss.
>
> I intentionally leave that as-is, because I think we should
> determine the updatability of a foreign table in the current
> manner. As you pointed out, even if the FDW doesn't provide eg,
> ExecForeignUpdate, an UPDATE on a foreign table could be done using
> the DML pushdown APIs if the UPDATE is *pushdown-safe*. However,
> since all UPDATEs on the foreign table are not necessarily
> pushdown-safe, I'm not sure it's a good idea to assume the
> table-level updatability if the FDW provides the DML pushdown
> callback routines. To keep the existing updatability decision, I
> think the FDW should provide the DML pushdown callback routines
> together with ExecForeignInsert, ExecForeignUpdate, or
> ExecForeignDelete. What do you think about that?

> Sorry but I am not in favour of adding compulsion that FDW should provide
> the DML pushdown callback routines together with existing ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete APIs.
>
> May be we should change the documentation in such way, that explains
>
> a) If FDW PlanDMLPushdown is NULL, then check for ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete APIs
> b) If FDW PlanDMLPushdown is non-NULL and plan is not pushable
> check for ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete APIs
> c) If FDW PlanDMLPushdown is non-NULL and plan is pushable
> check for DMLPushdown APIs.
>
> Does this sounds wired ?

Yeah, but I think that that would be what is done during executor
startup (see CheckValidResultRel()), while what the documentation is
saying is about relation_is_updatable(); that is, how to decide the
updatability of a given foreign table, not how the executor processes an
individual INSERT/UPDATE/DELETE on a updatable foreign table. So, I'm
not sure it's a good idea to modify the documentation in such a way.

BTW, I have added the description about that check partially. I added
to the PlanDMLPushdown documentation:

+ If this function succeeds, <function>PlanForeignModify</>
+ won't be executed, and <function>BeginDMLPushdown</>,
+ <function>IterateDMLPushdown</> and <function>EndDMLPushdown</> will
+ be called at the execution stage, instead.

And for example, I added to the BeginDMLPushdown documentation:

+ If the <function>BeginDMLPushdown</> pointer is set to
+ <literal>NULL</>, attempts to execute the table update directly on
+ the remote server will fail with an error message.

However, I agree that we should add a documentation note about the
compulsion somewhere. Maybe something like this:

The FDW should provide DML pushdown callback routines together with
table-updating callback routines described above. Even if the callback
routines are provided, the updatability of a foreign table is determined
based on the presence of ExecForeignInsert, ExecForeignUpdate or
ExecForeignDelete if the IsForeignRelUpdatable pointer is set to NULL.

What's your opinion?

Best regards,
Etsuro Fujita

On 2016/01/27 12:20, Etsuro Fujita wrote:
> On 2016/01/26 22:57, Rushabh Lathia wrote:
>> On Tue, Jan 26, 2016 at 4: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/25 17:03, Rushabh Lathia wrote:

>> int
>> IsForeignRelUpdatable (Relation rel);

>> Documentation for IsForeignUpdatable() need to change as it says:
>>
>> If the IsForeignRelUpdatable pointer is set to NULL, foreign
>> tables are
>> assumed
>> to be insertable, updatable, or deletable if the FDW provides
>> ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete respectively.
>>
>> With introduce of DMLPushdown API now this is no more correct,
>> as even if
>> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
>> ExecForeignDelete API
>> still foreign tables are assumed to be updatable or deletable
>> with
>> DMLPushdown
>> API's, right ?

>> That's what I'd like to discuss.
>>
>> I intentionally leave that as-is, because I think we should
>> determine the updatability of a foreign table in the current
>> manner. As you pointed out, even if the FDW doesn't provide eg,
>> ExecForeignUpdate, an UPDATE on a foreign table could be done using
>> the DML pushdown APIs if the UPDATE is *pushdown-safe*. However,
>> since all UPDATEs on the foreign table are not necessarily
>> pushdown-safe, I'm not sure it's a good idea to assume the
>> table-level updatability if the FDW provides the DML pushdown
>> callback routines. To keep the existing updatability decision, I
>> think the FDW should provide the DML pushdown callback routines
>> together with ExecForeignInsert, ExecForeignUpdate, or
>> ExecForeignDelete. What do you think about that?

>> Sorry but I am not in favour of adding compulsion that FDW should provide
>> the DML pushdown callback routines together with existing
>> ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete APIs.
>>
>> May be we should change the documentation in such way, that explains
>>
>> a) If FDW PlanDMLPushdown is NULL, then check for ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete APIs
>> b) If FDW PlanDMLPushdown is non-NULL and plan is not pushable
>> check for ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete APIs
>> c) If FDW PlanDMLPushdown is non-NULL and plan is pushable
>> check for DMLPushdown APIs.
>>
>> Does this sounds wired ?

> Yeah, but I think that that would be what is done during executor
> startup (see CheckValidResultRel()), while what the documentation is
> saying is about relation_is_updatable(); that is, how to decide the
> updatability of a given foreign table, not how the executor processes an
> individual INSERT/UPDATE/DELETE on a updatable foreign table. So, I'm
> not sure it's a good idea to modify the documentation in such a way.

> However, I agree that we should add a documentation note about the
> compulsion somewhere. Maybe something like this:
>
> The FDW should provide DML pushdown callback routines together with
> table-updating callback routines described above. Even if the callback
> routines are provided, the updatability of a foreign table is determined
> based on the presence of ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete if the IsForeignRelUpdatable pointer is set to NULL.

On second thought, I think it might be okay to assume the presence of
PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown, and
EndDMLPushdown is also sufficient for the insertablity, updatability,
and deletability of a foreign table, if the IsForeignRelUpdatable
pointer is set to NULL. How about modifying the documentation like this:

If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
assumed to be insertable, updatable, or deletable if the FDW provides
ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete respectively,
or if the FDW provides PlanDMLPushdown, BeginDMLPushdown,
IterateDMLPushdown, and EndDMLPushdown described below.

Of course, we also need to modify relation_is_updatable() accordingly.

What's your opinion?

Best regards,
Etsuro Fujita

On Wed, Jan 27, 2016 at 2:50 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2016/01/27 12:20, Etsuro Fujita wrote:
>
>> On 2016/01/26 22:57, Rushabh Lathia wrote:
>>
>>> On Tue, Jan 26, 2016 at 4: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/25 17:03, Rushabh Lathia wrote:
>>>
>>
> int
>>> IsForeignRelUpdatable (Relation rel);
>>>
>>
> Documentation for IsForeignUpdatable() need to change as it says:
>>>
>>> If the IsForeignRelUpdatable pointer is set to NULL, foreign
>>> tables are
>>> assumed
>>> to be insertable, updatable, or deletable if the FDW provides
>>> ExecForeignInsert,
>>> ExecForeignUpdate or ExecForeignDelete respectively.
>>>
>>> With introduce of DMLPushdown API now this is no more correct,
>>> as even if
>>> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
>>> ExecForeignDelete API
>>> still foreign tables are assumed to be updatable or deletable
>>> with
>>> DMLPushdown
>>> API's, right ?
>>>
>>
> That's what I'd like to discuss.
>>>
>>> I intentionally leave that as-is, because I think we should
>>> determine the updatability of a foreign table in the current
>>> manner. As you pointed out, even if the FDW doesn't provide eg,
>>> ExecForeignUpdate, an UPDATE on a foreign table could be done using
>>> the DML pushdown APIs if the UPDATE is *pushdown-safe*. However,
>>> since all UPDATEs on the foreign table are not necessarily
>>> pushdown-safe, I'm not sure it's a good idea to assume the
>>> table-level updatability if the FDW provides the DML pushdown
>>> callback routines. To keep the existing updatability decision, I
>>> think the FDW should provide the DML pushdown callback routines
>>> together with ExecForeignInsert, ExecForeignUpdate, or
>>> ExecForeignDelete. What do you think about that?
>>>
>>
> Sorry but I am not in favour of adding compulsion that FDW should provide
>>> the DML pushdown callback routines together with existing
>>> ExecForeignInsert,
>>> ExecForeignUpdate or ExecForeignDelete APIs.
>>>
>>> May be we should change the documentation in such way, that explains
>>>
>>> a) If FDW PlanDMLPushdown is NULL, then check for ExecForeignInsert,
>>> ExecForeignUpdate or ExecForeignDelete APIs
>>> b) If FDW PlanDMLPushdown is non-NULL and plan is not pushable
>>> check for ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete APIs
>>> c) If FDW PlanDMLPushdown is non-NULL and plan is pushable
>>> check for DMLPushdown APIs.
>>>
>>> Does this sounds wired ?
>>>
>>
> Yeah, but I think that that would be what is done during executor
>> startup (see CheckValidResultRel()), while what the documentation is
>> saying is about relation_is_updatable(); that is, how to decide the
>> updatability of a given foreign table, not how the executor processes an
>> individual INSERT/UPDATE/DELETE on a updatable foreign table. So, I'm
>> not sure it's a good idea to modify the documentation in such a way.
>>
>
> However, I agree that we should add a documentation note about the
>> compulsion somewhere. Maybe something like this:
>>
>> The FDW should provide DML pushdown callback routines together with
>> table-updating callback routines described above. Even if the callback
>> routines are provided, the updatability of a foreign table is determined
>> based on the presence of ExecForeignInsert, ExecForeignUpdate or
>> ExecForeignDelete if the IsForeignRelUpdatable pointer is set to NULL.
>>
>
> On second thought, I think it might be okay to assume the presence of
> PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown, and EndDMLPushdown
> is also sufficient for the insertablity, updatability, and deletability of
> a foreign table, if the IsForeignRelUpdatable pointer is set to NULL. How
> about modifying the documentation like this:
>
> If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
> assumed to be insertable, updatable, or deletable if the FDW provides
> ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete respectively, or
> if the FDW provides PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown,
> and EndDMLPushdown described below.
>
> Of course, we also need to modify relation_is_updatable() accordingly.
>
>
> What's your opinion?
>
>
If I understood correctly, above documentation means, that if FDW have
DMLPushdown APIs that is enough. But in reality thats not the case, we need
ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete in case DML is
not pushable.

And here fact is DMLPushdown APIs are optional for FDW, so that if FDW
don't have DMLPushdown APIs they can still very well perform the DML
operations using ExecForeignInsert, ExecForeignUpdate, or
ExecForeignDelete. So documentation should be like:

If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
assumed to be insertable, updatable, or deletable if the FDW provides
ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete respectively,

If FDW provides DMLPushdown APIs and the DML are pushable to the foreign
server, then FDW still needs ExecForeignInsert, ExecForeignUpdate, or
ExecForeignDelete for the non-pushable DML operation.

What's your opinion ?

Best regards,
> Etsuro Fujita
>
>
>

--
Rushabh Lathia

On Thu, Jan 21, 2016 at 4:05 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> By the way, I'm not too sure I understand the need for the core
>> changes that are part of this patch, and I think that point merits
>> some discussion. Whenever you change core like this, you're changing
>> the contract between the FDW and core; it's not just postgres_fdw that
>> needs updating, but every FDW. So we'd better be pretty sure we need
>> these changes and they are adequately justified before we think about
>> putting them into the tree. Are these core changes really needed
>> here, or can we fix this whole issue in postgres_fdw and leave the
>> core code alone?
>
> Well, if we think it is the FDW's responsibility to insert a valid value for
> tableoid in the returned slot during ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete, we don't need those core changes. However, I think it
> would be better that that's done by ModifyTable in the same way as
> ForeignScan does in ForeignNext, IMO. That eliminates the need for
> postgres_fdw or any other FDW to do that business in the callback routines.

I'm not necessarily opposed to the core changes, but I want to
understand better what complexity they are avoiding. Can you send a
version of this patch that only touches postgres_fdw, so I can
compare?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016/01/27 21:23, Rushabh Lathia wrote:
> If I understood correctly, above documentation means, that if FDW have
> DMLPushdown APIs that is enough. But in reality thats not the case, we
> need ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete in case
> DML is not pushable.
>
> And here fact is DMLPushdown APIs are optional for FDW, so that if FDW
> don't have DMLPushdown APIs they can still very well perform the DML
> operations using ExecForeignInsert, ExecForeignUpdate, or
> ExecForeignDelete.

I agree with you. I guess I was wrong. sorry.

> So documentation should be like:
>
> If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
> assumed to be insertable, updatable, or deletable if the FDW provides
> ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete respectively,
>
> If FDW provides DMLPushdown APIs and the DML are pushable to the foreign
> server, then FDW still needs ExecForeignInsert, ExecForeignUpdate, or
> ExecForeignDelete for the non-pushable DML operation.
>
> What's your opinion ?

I agree that we should add this to the documentation, too.

BTW, if I understand correctly, I think we should also modify
relation_is_updatabale() accordingly. Am I right?

Best regards,
Etsuro Fujita

On Thu, Jan 28, 2016 at 11:33 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> On 2016/01/27 21:23, Rushabh Lathia wrote:
>
>> If I understood correctly, above documentation means, that if FDW have
>> DMLPushdown APIs that is enough. But in reality thats not the case, we
>> need ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete in case
>> DML is not pushable.
>>
>> And here fact is DMLPushdown APIs are optional for FDW, so that if FDW
>> don't have DMLPushdown APIs they can still very well perform the DML
>> operations using ExecForeignInsert, ExecForeignUpdate, or
>> ExecForeignDelete.
>>
>
> I agree with you. I guess I was wrong. sorry.
>
> So documentation should be like:
>>
>> If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
>> assumed to be insertable, updatable, or deletable if the FDW provides
>> ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete respectively,
>>
>> If FDW provides DMLPushdown APIs and the DML are pushable to the foreign
>> server, then FDW still needs ExecForeignInsert, ExecForeignUpdate, or
>> ExecForeignDelete for the non-pushable DML operation.
>>
>> What's your opinion ?
>>
>
> I agree that we should add this to the documentation, too.
>
> BTW, if I understand correctly, I think we should also modify
> relation_is_updatabale() accordingly. Am I right?
>

Yep, we need to modify relation_is_updatable().

>
> Best regards,
> Etsuro Fujita
>
>
>

--
Rushabh Lathia

On 2016/01/28 15:20, Rushabh Lathia wrote:
> On Thu, Jan 28, 2016 at 11:33 AM, 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/27 21:23, Rushabh Lathia wrote:
>
> If I understood correctly, above documentation means, that if
> FDW have
> DMLPushdown APIs that is enough. But in reality thats not the
> case, we
> need ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete
> in case
> DML is not pushable.
>
> And here fact is DMLPushdown APIs are optional for FDW, so that
> if FDW
> don't have DMLPushdown APIs they can still very well perform the DML
> operations using ExecForeignInsert, ExecForeignUpdate, or
> ExecForeignDelete.

> I agree with you. I guess I was wrong. sorry.
>
> So documentation should be like:
>
> If the IsForeignRelUpdatable pointer is set to NULL, foreign
> tables are
> assumed to be insertable, updatable, or deletable if the FDW
> provides
> ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete
> respectively,
>
> If FDW provides DMLPushdown APIs and the DML are pushable to the
> foreign
> server, then FDW still needs ExecForeignInsert,
> ExecForeignUpdate, or
> ExecForeignDelete for the non-pushable DML operation.
>
> What's your opinion ?

> I agree that we should add this to the documentation, too.
>
> BTW, if I understand correctly, I think we should also modify
> relation_is_updatabale() accordingly. Am I right?

> Yep, we need to modify relation_is_updatable().

OK, will do.

Best regards,
Etsuro Fujita