Re: Optimization for updating foreign tables in Postgres FDW

On 2016/01/28 15:20, Rushabh Lathia wrote:

On Thu, Jan 28, 2016 at 11:33 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

    On 2016/01/27 21:23, Rushabh Lathia wrote:

        If I understood correctly, above documentation means, that if
        FDW have
        DMLPushdown APIs that is enough. But in reality thats not the
        case, we
        need  ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete
        in case
        DML is not pushable.

        And here fact is DMLPushdown APIs are optional for FDW, so that
        if FDW
        don't have DMLPushdown APIs they can still very well perform the DML
        operations using ExecForeignInsert, ExecForeignUpdate, or
        ExecForeignDelete.

        So documentation should be like:

        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,

        If FDW provides DMLPushdown APIs and the DML are pushable to the
        foreign
        server, then FDW still needs ExecForeignInsert,
        ExecForeignUpdate, or
        ExecForeignDelete for the non-pushable DML operation.

        What's your opinion ?

    I agree that we should add this to the documentation, too.

I added docs to the IsForeignRelUpdatable documentation.  Also, a brief introductory remark has been added at the beginning of the DML pushdown APIs' documentation.

    BTW, if I understand correctly, I think we should also modify
    relation_is_updatabale() accordingly.  Am I right?

Yep, we need to modify relation_is_updatable().

I thought I'd modify that function in the same way as CheckValidResultRel(), but I noticed that we cannot do that, because we don't have any information on whether each update is pushed down to the remote server by PlanDMLPushdown, during relation_is_updatabale(). 

So, I left that function as-is.  relation_is_updatabale() is just used for display in the information_schema views, so ISTM that that function is fine as-is.  (As for CheckValidResultRel(), I revised it so as to check the presence of DML pushdown APIs after checking the existing APIs if the given command will be pushed down.  The reason is because we assume the presence of the existing APIs, anyway.)

I revised other docs and some comments, mostly for consistency.

Attached is an updated version of the patch, which has been created on top of the updated version of the bugfix patch posted by Robert in [1] (attached).

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/CA+TgmoZ40j2uC5aC1NXu03oj4CrVOLkS15XX+PTFP-1U-8zR1Q@mail.gmail.com