Re: Optimization for updating foreign tables in Postgres FDW - Mailing list pgsql-hackers

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