Re: Optimization for updating foreign tables in Postgres FDW

On 2016/02/22 20:13, Rushabh Lathia wrote:
> I did another round of review for the latest patch and well as performed
> the sanity test, and
> haven't found any functional issues. Found couple of issue, see in-line
> comments
> for the same.

Thanks!

> On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
>
>     On 2016/02/12 21:19, Etsuro Fujita wrote:

>             +       /* Check point 1 */
>             +       if (operation == CMD_INSERT)
>             +               return false;
>             +
>             +       /* Check point 2 */
>             +       if (nodeTag(subplan) != T_ForeignScan)
>             +               return false;
>             +
>             +       /* Check point 3 */
>             +       if (subplan->qual != NIL)
>             +               return false;
>             +
>             +       /* Check point 4 */
>             +       if (operation == CMD_UPDATE)
>
>             These comments are referring to something in the function header
>             further up, but you could instead just delete the stuff from the
>             header and mention the actual conditions here.

>         Will fix.

>     Done.
>
>     The patch doesn't allow the postgres_fdw to push down an
>     UPDATE/DELETE on a foreign join, so I added one more condition here
>     not to handle such cases.  (I'm planning to propose a patch to
>     handle such cases, in the next CF.)

> I think we should place the checking foreign join condition before the
> target columns, as foreign join condition is less costly then the target
> columns.

Agreed.

>     Other changes:

>     * I keep Rushabh's code change that we call PlanDMLPushdown only
>     when all the required APIs are available with FDW, but for
>     CheckValidResultRel, I left the code as-is (no changes to that
>     function), to match the docs saying that the FDW needs to provide
>     the DML pushdown callback functions together with existing
>     table-updating functions such as ExecForeignInsert,
>     ExecForeignUpdate and ExecForeignDelete.

> I think we should also update the CheckValidResultRel(), because even
> though ExecForeignInsert,
> ExecForeignUpdate and ExecForeignDelete not present, FDW still can perform
> UPDATE/DELETE/INSERT using DML Pushdown APIs. Lets take committer's view
> on this.

OK

> PFA update patch, which includes changes into postgresPlanDMLPushdown()
> to check for join
> condition before target columns and also fixed couple of whitespace issues.

Thanks again for updating the patch and fixing the issues!

Best regards,
Etsuro Fujita