Re: Optimization for updating foreign tables in Postgres FDW

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