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

On 2016/01/26 22:57, Rushabh Lathia wrote:
> On Tue, Jan 26, 2016 at 4:15 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
>
>     On 2016/01/25 17:03, Rushabh Lathia wrote:

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

>     That's what I'd like to discuss.
>
>     I intentionally leave that as-is, because I think we should
>     determine the updatability of a foreign table in the current
>     manner.  As you pointed out, even if the FDW doesn't provide eg,
>     ExecForeignUpdate, an UPDATE on a foreign table could be done using
>     the DML pushdown APIs if the UPDATE is *pushdown-safe*.  However,
>     since all UPDATEs on the foreign table are not necessarily
>     pushdown-safe, I'm not sure it's a good idea to assume the
>     table-level updatability if the FDW provides the DML pushdown
>     callback routines.  To keep the existing updatability decision, I
>     think the FDW should provide the DML pushdown callback routines
>     together with ExecForeignInsert, ExecForeignUpdate, or
>     ExecForeignDelete.  What do you think about that?

> Sorry but I am not in favour of adding compulsion that FDW should provide
> the DML pushdown callback routines together with existing ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete APIs.
>
> May be we should change the documentation in such way, that explains
>
> a) If FDW PlanDMLPushdown is NULL, then check for ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete APIs
> b) If FDW PlanDMLPushdown is non-NULL and plan is not pushable
> check for ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete APIs
> c) If FDW PlanDMLPushdown is non-NULL and plan is pushable
> check for DMLPushdown APIs.
>
> Does this sounds wired ?

Yeah, but I think that that would be what is done during executor 
startup (see CheckValidResultRel()), while what the documentation is 
saying is about relation_is_updatable(); that is, how to decide the 
updatability of a given foreign table, not how the executor processes an 
individual INSERT/UPDATE/DELETE on a updatable foreign table.  So, I'm 
not sure it's a good idea to modify the documentation in such a way.

BTW, I have added the description about that check partially.  I added 
to the PlanDMLPushdown documentation:

+      If this function succeeds, <function>PlanForeignModify</>
+      won't be executed, and <function>BeginDMLPushdown</>,
+      <function>IterateDMLPushdown</> and <function>EndDMLPushdown</> will
+      be called at the execution stage, instead.

And for example, I added to the BeginDMLPushdown documentation:

+      If the <function>BeginDMLPushdown</> pointer is set to
+      <literal>NULL</>, attempts to execute the table update directly on
+      the remote server will fail with an error message.

However, I agree that we should add a documentation note about the 
compulsion somewhere.  Maybe something like this:

The FDW should provide DML pushdown callback routines together with 
table-updating callback routines described above.  Even if the callback 
routines are provided, the updatability of a foreign table is determined 
based on the presence of ExecForeignInsert, ExecForeignUpdate or 
ExecForeignDelete if the IsForeignRelUpdatable pointer is set to NULL.

What's your opinion?

Best regards,
Etsuro Fujita