Re: Optimization for updating foreign tables in Postgres FDW

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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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
www.EnterpriseDB.com