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