Re: Optimization for updating foreign tables in Postgres FDW

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
Cc: Thom Brown <thom(at)linux(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Optimization for updating foreign tables in Postgres FDW
Date: 2016-02-18 09:45:47
Message-ID: 56C592CB.8040807@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
fdw-dml-pushdown-v8.patch application/x-patch 94.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andreas Joseph Krogh 2016-02-18 09:46:12 Re: JDBC behaviour
Previous Message Sridhar N Bamandlapally 2016-02-18 09:42:37 Re: JDBC behaviour