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
onlaurenz.rtest (cost=100.00..14134.40 rows=299970 width=10) (actual time=0.005..0.005 rows=0 loops=1) -> Foreign
Scanon 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
msExecutiontime: 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
