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

On 2016/02/12 21:19, Etsuro Fujita wrote:

Comments on specific points follow.

This seems to need minor rebasing in the wake of the join pushdown patch.

Will do.

Done.

+       /* Likewise for ForeignScan that has pushed down
INSERT/UPDATE/DELETE */
+       if (IsA(plan, ForeignScan) &&
+               (((ForeignScan *) plan)->operation == CMD_INSERT ||
+                ((ForeignScan *) plan)->operation == CMD_UPDATE ||
+                ((ForeignScan *) plan)->operation == CMD_DELETE))
+               return;

I don't really understand why this is a good idea.  Since target lists
are only displayed with EXPLAIN (VERBOSE), I don't really understand
what is to be gained by suppressing them in any case at all, and
therefore I don't understand why it's a good idea to do it here,
either.  If there is a good reason, maybe the comment should explain
what it is.

I think that displaying target lists would be confusing for users.

There seems no objection from you (or anyone), I left that as proposed, and added more comments.

+       /* 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.)

- If the first condition is supposed accept only CMD_UPDATE or
CMD_DELETE, say if (operation != CMD_UPDATE || operation !=
CMD_DELETE) rather than doing it this way.  Is-not-insert may in this
context be functionally equivalent to is-update-or-delete, but it's
better to write the comment and the code so that they exactly match
rather than kinda-match.

- For point 2, use IsA(subplan, ForiegnScan).

Will fix.

Done.

+                       /*
+                        * ignore subplan if the FDW pushes down the
command to the remote
+                        * server
+                        */

This comment states what the code does, instead of explaining why it
does it.  Please update it so that it explains why it does it.

Will update.

Done.

+       List       *fdwPushdowns;       /* per-target-table FDW
pushdown flags */

Isn't a list of booleans an awfully inefficient representation?  How
about a Bitmapset?

OK, will do.

Done.

+       /*
+        * Prepare remote-parameter expressions for evaluation.
(Note: in
+        * practice, we expect that all these expressions will be just
Params, so
+        * we could possibly do something more efficient than using
the full
+        * expression-eval machinery for this.  But probably there
would be little
+        * benefit, and it'd require postgres_fdw to know more than is
desirable
+        * about Param evaluation.)
+        */
+       dpstate->param_exprs = (List *)
+               ExecInitExpr((Expr *) fsplan->fdw_exprs,
+                                        (PlanState *) node);

This is an exact copy of an existing piece of code and its associated
comment.  A good bit of the surrounding code is copied, too.  You need
to refactor to avoid duplication, like by putting some of the code in
a new function that both postgresBeginForeignScan and
postgresBeginForeignModify can call.

execute_dml_stmt() has some of the same disease.

Will do.

Done.

Other changes:

* I fixed docs as discussed before with Rushabh Lathia and Thom Brown.

* 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.