Re: Optimization for updating foreign tables in Postgres FDW - Mailing list pgsql-hackers

From Rushabh Lathia
Subject Re: Optimization for updating foreign tables in Postgres FDW
Date
Msg-id CAGPqQf1mBg_fN9LLyXhaYb7-r9DdA=RXLZhcG_EZK3wjup6W6Q@mail.gmail.com
Whole thread Raw
In response to Re: Optimization for updating foreign tables in Postgres FDW  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
Responses Re: Optimization for updating foreign tables in Postgres FDW
Re: Optimization for updating foreign tables in Postgres FDW
List pgsql-hackers

I did another round of review for the latest patch and well as performed the sanity test, and
haven't found any functional issues. Found couple of issue, see in-line comments
for the same.

On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
On 2016/02/12 21:19, Etsuro Fujita wrote:
Comments on specific points follow.

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

Will do.

Done.

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

There seems no objection from you (or anyone), I left that as proposed, and added more comments.

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

Will fix.

Done.

The patch doesn't allow the postgres_fdw to push down an UPDATE/DELETE on a foreign join, so I added one more condition here not to handle such cases.  (I'm planning to propose a patch to handle such cases, in the next CF.)

I think we should place the checking foreign join condition before the
target columns, as foreign join condition is less costly then the target
columns.
 

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

Done.

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

Done.

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

Done.

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

Done.

Other changes:

* I fixed docs as discussed before with Rushabh Lathia and Thom Brown.

* I keep Rushabh's code change that we call PlanDMLPushdown only when all the required APIs are available with FDW, but for CheckValidResultRel, I left the code as-is (no changes to that function), to match the docs saying that the FDW needs to provide the DML pushdown callback functions together with existing table-updating functions such as ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete.


I think we should also update the CheckValidResultRel(), because even though ExecForeignInsert,
ExecForeignUpdate and ExecForeignDelete not present, FDW still can perform
UPDATE/DELETE/INSERT using DML Pushdown APIs. Lets take committer's view on this.

PFA update patch, which includes changes into postgresPlanDMLPushdown() to check for join
condition before target columns and also fixed couple of whitespace issues.



Regards
Rushabh Lathia
Attachment

pgsql-hackers by date:

Previous
From: Robert Haas
Date:
Subject: Re: postgres_fdw vs. force_parallel_mode on ppc
Next
From: Michael Paquier
Date:
Subject: Re: Writing new unit tests with PostgresNode