On 25 December 2015 at 10:00, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2015/12/24 4:34, Robert Haas wrote: >> >> On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia >> <rushabh.lat...@gmail.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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers