On Wed, Feb 10, 2016 at 3:00 AM, Rushabh Lathia <rushabh.lat...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
