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

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