Re: Optimization for updating foreign tables in Postgres FDW

On 2016/01/26 22:57, Rushabh Lathia wrote:
> On Tue, Jan 26, 2016 at 4:15 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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