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
