Re: Optimization for updating foreign tables in Postgres FDW

On Wed, Feb 10, 2016 at 3:00 AM, Rushabh Lathia
<rushabh(dot)lathia(at)gmail(dot)com> wrote:
> Fujita-san, I am attaching update version of the patch, which added
> the documentation update.
>
> Once we finalize this, I feel good with the patch and think that we
> could mark this as ready for committer.

It would be nice to hear from Tom and/or Stephen whether the changes
that have been made since they last commented on it. I feel like the
design is reasonably OK, but I don't want to push this through if
they're still not happy with it. One thing I'm not altogether keen on
is the use of "pushdown" or "dml pushdown" as a term of art; on the
other hand, I'm not sure what other term would be better.

Comments on specific points follow.

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

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

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

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

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

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

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

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

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company