Re: Optimization for updating foreign tables in Postgres FDW

On Thu, Jan 21, 2016 at 3:20 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> 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.
>

This looks great.

>
> 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
>

Here are couple of comments:

1)

int
IsForeignRelUpdatable (Relation rel);

Documentation for IsForeignUpdatable() need to change as it says:

If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
assumed
to be insertable, updatable, or deletable if the FDW provides
ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete respectively.

With introduce of DMLPushdown API now this is no more correct, as even if
FDW don't provide ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete
API
still foreign tables are assumed to be updatable or deletable with
DMLPushdown
API's, right ?

2)

+ /* SQL statement to execute remotely (as a String node) */
+ FdwDmlPushdownPrivateUpdateSql,

FdwDmlPushdownPrivateUpdateSql holds the UPDATE/DELETE query, so name should
be something like FdwDmlPushdownPrivateQuery os FdwDmlPushdownPrivateSql ?

Later I realized that for FdwModifyPrivateIndex too the index name is
FdwModifyPrivateUpdateSql even though its holding any DML query. Not sure
whether we should consider to change this or not ?

Apart from this perform sanity testing on the new patch and things working
as expected.

--
Rushabh Lathia
www.EnterpriseDB.com