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

On 2016/01/27 12:20, Etsuro Fujita wrote:

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.

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.

On second thought, I think it might be okay to assume the presence of PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown, and EndDMLPushdown is also sufficient for the insertablity, updatability, and deletability of a foreign table, if the IsForeignRelUpdatable pointer is set to NULL.  How about modifying the documentation like this:

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, or if the FDW provides PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown, and EndDMLPushdown described below.

Of course, we also need to modify relation_is_updatable() accordingly.

What's your opinion?

Best regards,
Etsuro Fujita