Re: Optimization for updating foreign tables in Postgres FDW

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

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

Best regards,
Etsuro Fujita