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