Hi Robert,

Thanks for the review!

On 2016/02/11 5:43, Robert Haas wrote:
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.

I'm open to that naming.  Proposals are welcome!

Comments on specific points follow.

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

Will do.

+       /* 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. Here is an example:

EXPLAIN (verbose, costs off)
DELETE FROM rem1;                 -- can be pushed down
                 QUERY PLAN
 Delete on public.rem1
   ->  Foreign Delete on public.rem1
         Output: ctid
         Remote SQL: DELETE FROM public.loc1
(4 rows)

Should we output the "Output" line?

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

Will fix.

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

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

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

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

Best regards,
Etsuro Fujita

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to