|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>|
|Cc:||Robert Haas <robertmhaas(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Optimization for updating foreign tables in Postgres FDW|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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:
> .) 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 , 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.
* 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
* Revise comments a bit further.
* Revise docs, including a fix for a wrong copy-and-paste.
|Next Message||Etsuro Fujita||2016-01-21 09:55:01||Re: Odd behavior in foreign table modification (Was: Re: Optimization for updating foreign tables in Postgres FDW)|
|Previous Message||Victor Wagner||2016-01-21 09:49:09||Re: Patch: Implement failover on libpq connect level.|