Re: Optimization for updating foreign tables in Postgres FDW

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