On 2015/12/24 4:34, Robert Haas wrote:
> On Wed, Dec 23, 2015 at 5:50 AM, Rushabh Lathia
> <rushabh.lathia@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.
Best regards,
Etsuro Fujita
