Re: Optimization for updating foreign tables in Postgres FDW

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
Date: 2016-01-21 09:50:40
Message-ID: 56A0A9F0.9090304@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: 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

Attachment Content-Type Size
fdw-dml-pushdown-v4.patch application/x-patch 92.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
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.